[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2314
+  nextToken();
+  if (FormatTok->Tok.is(tok::less)) {
+while (!FormatTok->Tok.is(tok::greater)) {

miscco wrote:
> I guess you could use `parseBracedList(/*ContinueOnSemicolons=*/false, 
> /*IsEnum=*/false,/*ClosingBraceKind=*/tok::greater);` here?
To be more specific, I am concerned what happens if you have multiple template 
arguments where a linebreak should occur. Is this still happening here?


```
template 
concept someVeryLongConceptName = someVeryLongConstraintName1;
```



Comment at: clang/unittests/Format/FormatTest.cpp:16319
+  verifyFormat("template \n"
+   "concept bool EqualityComparable = requires(T a, T b) {\n"
+   "  { a == b } -> bool;\n"

I think `concept bool` should just be `concept` occurs below 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added a comment.

Question: Should I add my wip work as a child revision or what would you suggest


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added a comment.

It seems the spacing of the binary operator is not yet stable. This test is 
breaking for me:

  verifyFormat("template \nconcept someConcept = Constraint1 && 
Constraint2;");


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2296
+
+void UnwrappedLineParser::parseRequiresExpression(int OriginalLevel) {
+  // requires (R range)

Line->Level is an unsigned int so this gives me a warning. I guess it should be 
`unsigned int OriginalLevel` same below


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67052: Add reference type transformation builtins

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:416
   static bool isTypeRep(TST T) {
-return (T == TST_typename || T == TST_typeofType ||
-T == TST_underlyingType || T == TST_atomic);
+return (TST_typename <= T <= TST_atomic);
   }

There is a bug here, this should probably be

```
return (TST_typename <= T && T <= TST_atomic);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67052/new/

https://reviews.llvm.org/D67052



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added a comment.

Awesome, Thank you very much, I dragged my feet on starting to implement 
something for real.

As a high level comment I think we need to handle requires expressions to get 
this correct, as `requires requires` would otherwise to bad things.

From my early brainstorming i got the following options I thought useful. This 
is in no ways a request for you to implement them, but I would want to get them 
out here in case they are usefull

  /// Different styles for merging short requires-clauses with the template
/// decalaration
enum ShortRequiresClauseStyle {
  /// Never merge requires clauses into a single line.
  /// \code
  ///   template 
  ///   requires someConstraint
  ///   T foo();
  /// \endcode
  SRCS_Never,
  /// Only merge requires clauses with a single constraint.
  /// \code
  ///   template  requires someConstraint
  ///   T foo();
  ///
  ///   template 
  ///   requires someConstraint && otherConstraint
  ///   T bar();
  /// \endcode
  SRCS_Single,
  /// Merge requires clauses if they are short
  /// \code
  ///   template  requires short && shorter
  ///   T foo();
  ///
  ///   template 
  ///   requires someReallyLongElaborateConstraintThatIsReallyLong
  ///   T bar();
  ///
  ///   template 
  ///   requires someLongConstraint && otherLongConstraint
  ///   T baz();
  /// \endcode
  SRCS_Short,
  /// Always try to merge the requires clause with the template declaration
  /// \code
  ///   template  requires someLongConstraint &&
  ///   otherLongConstraint
  ///   T foo();
  /// \endcode
  SRCS_Always,
};
  
/// Dependent on the value, requires-clauses can be put on the same line
/// as the template declaration.
ShortRequiresClauseStyle AllowShortRequiresClause;
  
/// Dependent on the value, requires-expressions can be a single line
/// Always try to merge the requires clause with the template declaration
/// \code
///   template  requires someLongConstraint &&
///   requires { T{}; }
///   T foo();
/// \endcode
bool AllowShortRequiresExpression;
  
 /// Dependent on the value break before or after constraint expressions.
enum ConstraintExpressionBreakingStyle {
  /// Break after constraint expressions but multiple may be on single line
  /// \code
  ///   template  requires someLongConstraint &&
  ///   otherLongConstraint
  ///   T foo();
  ///
  ///   template  requires short && shorter &&
  ///   otherLongConstraint
  ///   T bar();
  /// \endcode
  CEBS_After,
  /// Break after constraint expressions.
  /// \code
  ///   template  requires short &&
  ///   shorter &&
  ///   otherLongConstraint
  ///   T bar();
  /// \endcode
  CEBS_AfterSingleExpression,
  /// Break before constraint expressions but multiple may be on single line
  /// \code
  ///   template  requires someLongConstraint &&
  ///   otherLongConstraint
  ///   T foo();
  ///
  ///   template  requires short && shorter
  ///&& otherLongConstraint
  ///   T bar();
  /// \endcode
  CEBS_Before,
  /// Break before constraint expressions.
  /// \code
  ///   template  requires short
  ///&& shorter
  ///&& otherLongConstraint
  ///   T foo();
  /// \endcode
  CEBS_BeforeSingleExpression,
};
  
/// The constraint expressions style to use.
ConstraintExpressionBreakingStyle BreakBeforeConstraintExpression;
  
/// Dependent on the value wrap before or after requires expressions.
enum BraceWrappingRequiresExpressionStyle {
  /// Do not wrap braces of requires expressions
  /// \code
  ///   template  requires requires {  T{};
  ///   T(int); }
  ///   T foo();
  /// \endcode
  BWARES_NoWrap,
  /// Wrap after requires expressions.
  /// \code
  ///   template  requires requires { T{};
  ///  T(int);
  ///}
  ///   T foo();
  /// \endcode
  BWARES_WrapAfter,
  /// Wrap after requires expressions.
  /// \code
  ///   template  requires requires
  ///   { T{};
  /// T(int); }
  ///   T foo();
  /// \endcode
  BWARES_WrapBefore,
  /// Wrap after requires expressions with a new line.
  /// \code
  ///   template  requires requires
  ///   {

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:3499
 return true;
   if (Right.Previous->ClosesTemplateDeclaration &&
   Right.Previous->MatchingParen &&

I think that your change should actually come in here where we determine what 
to do after a `ClosesTemplateDeclaration` 

With an potential new option `AlwaysBreakConceptDeclarations ` that should 
probably default to `AlwaysBreakTemplateDeclarations ` this would read
```
  if (Right.Previous->ClosesTemplateDeclaration &&
  Right.Previous->MatchingParen &&
  Right.Previous->MatchingParen->NestingLevel == 0) { 
  if (Right.is(tok::kw_requires)) {
switch(Style.AllowShortRequiresClause) {
  case FormatStyle::SRCS_Never: 
return true;
  case FormatStyle::SRCS_Always: 
return false;
  case FormatStyle::SRCS_Single:
// TODO: Determine whether there is a single constraint 
return true;
  case FormatStyle::SRCS_Short: 
// TODO: Determine whether the constraint clause is short enough
return true;
} 
  } else if (Right.is(tok::kw_concept)) {
return Style.AlwaysBreakConceptDeclarations == FormatStyle::BTCS_Yes);
  } else {
return Style.AlwaysBreakTemplateDeclarations == FormatStyle::BTDS_Yes);
  }
  }
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2281
+  if (FormatTok->Tok.is(tok::kw_requires))
+parseRequires();
+}

I believe this should be `parseConstraintExpression`  because that is the term 
of art in the standard.

The requires expression is what is the `requieres { }` and that can be part of 
an constraint expression 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added a comment.

You are missing to check the boolean in `CHECK_PARSE_BOOL` in FormatTest.cpp




Comment at: clang/lib/Format/TokenAnnotator.cpp:1566
   Current.Type = TT_TrailingReturnArrow;
-
+} else if (Current.is(tok::arrow) && Current.Previous &&
+   Current.Previous->is(tok::r_brace)) {

MyDeveloperDay wrote:
> miscco wrote:
> > Should that really be `tok::arrow` and not `tok::kw_requires`?
> This is to prevent the breaking between } and -> in the following (which 
> could be some way from the requires
> 
> `{ t.foo(u) } -> typename T::foo_type;`
I believe this should carry some state that we are inside of an 
requires-expression. 



Comment at: clang/lib/Format/TokenAnnotator.cpp:3484
+  // concept ...
+  if (Left.is(TT_TemplateCloser) && Right.is(tok::kw_concept))
+return true;

MyDeveloperDay wrote:
> miscco wrote:
> > I think we should have an option to allow short requires clauses on the 
> > same line similar to allow short functions
> Actually at present it will pull short requires onto a new line, this 
> particular line is about keeping `concept` on a new line (most examplesI've 
> seen are like this..i.e.
> 
> ```template
> concept...```
> 
> rather than 
> 
> `template concept...`
> 
> For me the natural thing here is to enforce a newline and then relax that 
> later if we think it needs it, but I'm wary of adding too many options 
> upfront. 
> 
> This is virgin territory as most style guides out there don't specify 
> anything for concepts yet, to be honest i'd prefer to establish an LLVM style 
> and wait for the other style guides to catch up. (I know we will revisit this 
> later), just want to keep the initial commit small. (incremental development 
> is encouraged in LLVM) and multiple options will be a huge commit that never 
> gets passed.
> 
> 
Personally I agree that this is the natural style, at the same time there are 
configurations that allow short template declarations on a single line, which I 
believe would be similarly welcome. 


That said I agree adding this later is fine



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2284
+
+void UnwrappedLineParser::parseRequires() {
+  assert(FormatTok->Tok.is(tok::kw_requires) && "'requires' expected");

MyDeveloperDay wrote:
> miscco wrote:
> > I believe we should separate between constraint expressions aka: `tempalte< 
> >  > requires Concept1 && Concept2` and requires expressions aka `requires { 
> > ... }`
> > 
> > The two are quite different. That said the handling of requires expressions 
> > should be "simple" as we only need to find the matching "}". 
> > 
> > As an upside this would also handle the famous `requires requires`
> This is effectively the `if` at line 2305 seeing "requires {" or requires(Foo 
> t) {" sets off down the parseBlock path which can itself include requires.
> 
> If you do have an example you think would break this please feel free to add 
> that in a comment and I'll add it to the unit tests and rework accordingly.
> 
> My exposure to concepts is still pretty new, I'm not even sure I've covered 
> all the places it can be used, but I wanted to start because at present I see 
> alot of the MS STL(@STL_MSFT ) covered with // clang-format off around the 
> concepts.
Yes that was also the reason I tried it out because I often added those // 
clang-format off snippets once too often 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2314
+  nextToken();
+  if (FormatTok->Tok.is(tok::less)) {
+while (!FormatTok->Tok.is(tok::greater)) {

I guess you could use `parseBracedList(/*ContinueOnSemicolons=*/false, 
/*IsEnum=*/false,/*ClosingBraceKind=*/tok::greater);` here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79773: [clang-format] Improve clang-formats handling of concepts

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added a comment.

As someone who has extensivly worked with conscepts I cannot stress how much 
this would improve my live


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79773/new/

https://reviews.llvm.org/D79773

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73138: [libcxx] [test] Correct asserted type in subspan test; subspan with count should never produce dynamic_extent

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added a comment.

Ah sorry about that, I overlooked that we had special cased tests without count.

Thanks Billy


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73138/new/

https://reviews.llvm.org/D73138



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67052: Add reference type transformation builtins

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added subscribers: cjdb, miscco.
miscco added a comment.

Some nits, looks great, especially for library concepts. Pinging @cjdb




Comment at: clang/lib/AST/TypePrinter.cpp:1020
 
   switch (T->getUTTKind()) {
 case UnaryTransformType::EnumUnderlyingType:

Couldn't we use `TextNodeDumper::VisitUnaryTransformType` to dump the string 
and then simplify it to a single case.

Given the possibility that maybe some day one might add the `add_foo` builtins



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1110
+  if (T.expectAndConsume(diag::err_expected_lparen_after,
+ "reference manipulation builtin",
+ tok::r_paren)) return;

I dont think that this message is valid anymore now that you also remove 
qualifiers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67052/new/

https://reviews.llvm.org/D67052



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69520: [libc++] Disallow dynamic -> static span conversions

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added a comment.

I believe this is superseded by the implementation of P1976R2 in D74577 



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69520/new/

https://reviews.llvm.org/D69520



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100057: Remove warning "suggest braces" for aggregate initialization of an empty class with an aggregate base class.

[Michael Schellenberger Costa via Phabricator via cfe-commits]

miscco added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:1013
 
-  auto *ParentRD =
-  Entity.getParent()->getType()->castAs()->getDecl();
-  if (CXXRecordDecl *CXXRD = dyn_cast(ParentRD))
-if (CXXRD->getNumBases())
-  return false;
+  // Allows elide brace initialization for aggreagates with empty base
+  if (Entity.getKind() == InitializedEntity::EK_Base) {

There is a typo here `aggreagates` -> `aggregates`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100057/new/

https://reviews.llvm.org/D100057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits