[PATCH] D50360: [Concepts] Requires Expressions

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

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/ExprConcepts.h:144-148
+  struct SubstitutionDiagnostic {
+    StringRef SubstitutedEntity;
+    SourceLocation DiagLoc;
+    StringRef DiagMessage;
+  };
----------------
Please add a FIXME to store the diagnostic semantically rather than as a pre-rendered string.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3345
+  ParseScope BodyScope(this, Scope::DeclScope);
+  RequiresExprBodyDecl *Body = Actions.ActOnEnterRequiresExpr(
+      RequiresKWLoc, LocalParameterDecls, getCurScope());
----------------
Please call these `ActOnStart...` and `ActOnFinish...` for consistency with other such functions in `Sema`.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3348
+
+  if (Tok.is(tok::r_brace))
+    // Grammar does not allow an empty body.
----------------
This `if` body is long (even though it's just one statement); please add braces here.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3392
+        if (!TryConsumeToken(tok::arrow))
+          // User probably forgot the arrow, remind him and try to continue
+          Diag(Tok, diag::err_requires_expr_missing_arrow)
----------------
him -> them, and please add a period.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3447
+              if (Res != TPResult::False) {
+                // Skip to the closing parenthesis
+                unsigned Depth = 1;
----------------
Please add a FIXME to avoid walking these tokens twice (once in `TryParseParameterDeclarationClause` and again here).


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3504-3513
+          if (Tok.is(tok::annot_cxxscope)) {
+            // The lack of typename might have prevented this from being
+            // annotated as a type earlier.
+            UnconsumeToken(TypenameKW);
+            if (TryAnnotateTypeOrScopeToken()) {
+              SkipUntil(tok::semi, tok::r_brace,
+                        SkipUntilFlags::StopBeforeMatch);
----------------
Doing this by messing with the token stream and trying to perform annotation twice seems quite unclean, and you're doing too much of the semantic work of interpreting the typename requirement from inside the parser. Here's what I'd suggest:

First, call `TryAnnotateCXXScopeToken()`, which will annotate a scope and a following //template-id// (if any)

If we then have 
  * optionally, an `annot_cxxscope`, then
  * an `identifier` or an `annot_template_id`, then
  * a `semi` (or, not an `l_brace` nor an `l_paren`, depending on how you want to recover from errors)
then we have a //typename-requirement//. Call `Sema::ActOnTypeRequirement` and pass in the information you have (a scope and an identifier or template-id), and build a suitable type representation from `Sema`. (You might want to always build an `ElaboratedType` with `ETK_Typename`, even if the nested name specifier is non-dependent, to match the source syntax.)

And do this all in a tentative parse action, so you can revert it to put the `typename` keyword back when you find out that this is actually a //simple-requirement//.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3557
+          else
+            Diag(Tok, diag::err_requires_expr_simple_requirement_unexpected_tok)
+                << Tok.getKind() << FixItHint::CreateInsertion(StartLoc, "{")
----------------
Is it useful to suggest a //compound-requirement// here? I expect this'll be hit a lot more for normal typos in the expression (eg, missing semicolon, forgotten operator, etc) than in cases where a compound-requirement was intended. `ExpectAndConsumeSemi` has various tricks to diagnose these cases well, and should be expected to gain more such tricks in the future; you should just call it here unless you're confident you know what the user did wrong (eg, in the `noexcept` case).


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3574
+      // Don't emit an empty requires expr here to avoid confusing the user with
+      // other diagnostics quoting an empty requires expression he never wrote.
+      Braces.consumeClose();
----------------
he -> they


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3582
+  Actions.ActOnExitRequiresExpr();
+  return Actions.CreateRequiresExpr(RequiresKWLoc, Body, LocalParameterDecls,
+                                    Requirements, Braces.getCloseLocation());
----------------
This should be called `ActOn...` not `Create...`.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:402
+  if (Req->isSubstitutionFailure()) {
+    auto *SubstDiag = Req->getSubstitutionDiagnostic();
+    if (!SubstDiag->DiagMessage.empty())
----------------
Please don't use `auto` here; the type is not clear from context.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:964-968
+                /*IsSatisfied=*/true // We reach this ctor with either dependent
+                                     // types (in which IsSatisfied doesn't
+                                     // matter) or with non-dependent type in
+                                     // which the existance of the type
+                                     // indicates satisfaction.
----------------
Please move this long comment to the previous line. Long trailing comments are a pain to work with and harder to read.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:353-354
 
+  if (isa<ParmVarDecl>(D) && isa<RequiresExprBodyDecl>(D->getDeclContext()) &&
+      !isUnevaluatedContext()) {
+    // C++ [expr.prim.req.nested] p3
----------------
Please make sure you have a test for this in the `typeid` case (which we parse as unevaluated and then might transform to potentially-evaluated if the operand is an lvalue of polymorphic class type), eg:

```
class X { virtual ~X(); };
requires (X &x) { static_cast<int(*)[(typeid(x), 0)]>(nullptr); }
```


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8290-8293
+      // Verify that the argument identifier has not already been mentioned.
+      if (!ParamsSoFar.insert(Param->getIdentifier()).second)
+        Diag(Param->getBeginLoc(), diag::err_param_redefinition)
+            << Param->getIdentifier();
----------------
I'm surprised this is necessary; do we not already check for redefinition when building the `ParmVarDecl` and pushing it into scope?


================
Comment at: clang/lib/Sema/TreeTransform.h:11275-11277
+  for (concepts::Requirement *Req : TransReqs)
+    if (auto *ER = dyn_cast<concepts::ExprRequirement>(Req))
+      if (ER->getReturnTypeRequirement().isTypeConstraint())
----------------
Add braces here please.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50360