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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:179-180
 def err_unexpected_semi : Error<"unexpected ';' before %0">;
+def err_expected_primary_got_unary : Error<
+  "expected primary expression before %0; did you forget parentheses?">;
+def note_potential_bin_op_in_constraint_logical_or : Note<
----------------
I think the "did you forget parentheses?" part here may be irritating to users who didn't know they needed parentheses at all -- it may be read as patronizing by some. Can we rephrase as "parentheses are required around a top-level unary %0 expression in a requires clause" or something like that?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2515
+  "'%0' in the two declarations is not considered equivalent - move it to a "
+  "concept and reference it from here:">;
+def note_ambiguous_atomic_constraints_second : Note<"and here">;
----------------
Don't use a colon to indicate a location. Diagnostics are formatted in many different ways and this won't always make sense. Also, where possible, don't pretty-print expressions in diagnostics; underline the source range instead.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2516
+  "concept and reference it from here:">;
+def note_ambiguous_atomic_constraints_second : Note<"and here">;
 
----------------
Similarly, this note presentation won't always make sense. Can we somehow rephrase these notes so they're more presenting the facts and less telling the user what to do?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3886-3895
+def note_ovl_candidate_constraints_not_satisfied : Note<
+    "candidate %select{function|function|constructor|"
+    "function|function|constructor|"
+    "constructor (the implicit default constructor)|"
+    "constructor (the implicit copy constructor)|"
+    "constructor (the implicit move constructor)|"
+    "function (the implicit copy assignment operator)|"
----------------
Don't repeat this `%select`; use `%sub{select_ovl_candidate_kind}` instead. (The enum and the appropriate `%select` have already changed at least twice since this was copy-pasted...)


================
Comment at: clang/include/clang/Parse/Parser.h:1652
                                         prec::Level MinPrec);
-  ExprResult ParseCastExpression(bool isUnaryExpression,
+  /// CastParseOption - Control what ParseCastExpression will parse.
+  enum CastParseOption {
----------------
Don't repeat the name of the entity in the doc comment. (This is an old style that we're phasing out; new code shouldn't do it.)


================
Comment at: clang/include/clang/Parse/Parser.h:1653
+  /// CastParseOption - Control what ParseCastExpression will parse.
+  enum CastParseOption {
+    AnyCastExpr = 0,
----------------
We would usually call this sort of enumeration `SomethingKind` not `SomethingOption`.


================
Comment at: clang/include/clang/Parse/Parser.h:1658
+  };
+  ExprResult ParseCastExpression(CastParseOption ExprType,
                                  bool isAddressOfOperand,
----------------
Please don't use `Type` as the name of something that's not a language-level type.


================
Comment at: clang/include/clang/Parse/Parser.h:2700
+  void InitCXXThisScopeForDeclaratorIfRelevant(const Declarator &D,
+      const DeclSpec &DS, llvm::Optional<Sema::CXXThisScopeRAII> &ThisScope);
   bool ParseRefQualifier(bool &RefQualifierIsLValueRef,
----------------
Line-wrapped parameters should start on the same column as the first parameter.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2068-2069
 
   if (ParseAsmAttributesAfterDeclarator(D))
     return nullptr;
 
----------------
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.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2257-2258
+  //        declarator requires-clause
+  if (Tok.is(tok::kw_requires))
+    ParseTrailingRequiresClause(D);
+
----------------
We already parsed this for the first declarator in the group. Do we accidentally permit two requires clauses on the first declarator?


================
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())
----------------
This scope-building code should be in `Sema`, not in the parser. (Consider adding an `ActOnStartTrailingRequiresClause` / `ActOnFinishTrailingRequiresClause` pair.)


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3801
+      auto &FTI = D.getFunctionTypeInfo();
+      if (FTI.Params)
+        for (auto &Param : ArrayRef<DeclaratorChunk::ParamInfo>(FTI.Params,
----------------
Add braces for this multi-line `if`.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3825
+  if (TrailingRequiresClause.isInvalid())
+    SkipUntil({tok::equal, tok::l_brace, tok::arrow, tok::kw_try, tok::comma},
+              StopAtSemi | StopBeforeMatch);
----------------
Also `tok::colon` for constructors.


================
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:
----------------
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.)


================
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
----------------
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.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:901
+      if (Tok.is(tok::annot_typename))
+        Diag(Tok, diag::err_expected_primary_got_unary) << "type name";
+      else
----------------
Do not hardcode English strings into diagnostic arguments. Use a `%select` or a different diagnostic instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43357