[PATCH] D43357: [Concepts] Function trailing requires clauses

[Saar Raz via Phabricator via cfe-commits]

saar.raz added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2068-2069
 
   if (ParseAsmAttributesAfterDeclarator(D))
     return nullptr;
 
----------------
rsmith wrote:
> We should check with GCC to see which side of the requires-clause they expect this.
> 
> For the second and subsequent declarations, we expect the asm label before the requires clause; here we parse the asm label after the requires clause.
They expect the requires clause first.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3799-3810
+    if (D.isFunctionDeclarator()) {
+      auto &FTI = D.getFunctionTypeInfo();
+      if (FTI.Params)
+        for (auto &Param : ArrayRef<DeclaratorChunk::ParamInfo>(FTI.Params,
+                                                                FTI.NumParams)){
+          auto *ParamDecl = cast<NamedDecl>(Param.Param);
+          if (ParamDecl->getIdentifier())
----------------
rsmith wrote:
> This scope-building code should be in `Sema`, not in the parser. (Consider adding an `ActOnStartTrailingRequiresClause` / `ActOnFinishTrailingRequiresClause` pair.)
Is there anything that needs to be in ActOnFinishTrailingRequiresClause?


================
Comment at: clang/lib/Parse/ParseExpr.cpp:349-350
+  if (Tok.is(tok::l_paren) &&
+      isa<UnresolvedLookupExpr>(RightMostExpr) &&
+      RightMostExpr->isTypeDependent()) {
+    // We're facing one of the following cases:
----------------
rsmith wrote:
> The parser shouldn't be encoding this kind of semantic knowledge. Please move this to `Sema`. (This is also not really a very general way to detect a function-like name: checking for an expression whose type is a function type or OverloadType would catch more cases.)
Function types and overload types would not reach here (would be eliminated in the previous check for 'bool' types). Only dependent types and bools get here, and checking for a function type or OverloadType would not catch those cases.

Anyway, moved this to Sema


================
Comment at: clang/lib/Parse/ParseExpr.cpp:354
+    // template<typename> void foo() requires func<T>(
+    // In the first case, '(' cannot start a declaration, and in the second,
+    // '(' cannot follow the requires-clause in a function-definition nor in
----------------
rsmith wrote:
> I don't think this is true. Consider:
> 
> ```
> struct A {
>   template<typename T> requires X<T>
>   (A)() {}
> };
> ```
> 
> For a trailing requires clause, we can be much more aggressive with our error detection here, though, and in that case perhaps we can unconditionally treat a trailing `(` as a function call.
If the cases where this is possible are only very remote - could we maybe turn this into a warning instead and keep it here?

Also, if X<T> is a function type or OverloadType, then this is ill-formed anyway, right? since the constraint expression can't be a bool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43357