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

Wed Jan 8 16:05:48 PST 2020

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks, I'm happy to iterate on this further after you commit (though feel free to ask for another round of review if you prefer). The changes to recovery from missing parens in a trailing requires clause can be done in a separate change after this one if you like.

================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:186-188
+def err_unparenthesized_typename_expr_in_requires_clause : Error<
+  "parentheses are required around initialization expression in a requires "
+  "clause">;
----------------
This diagnostic is now unused; please remove it.

================
Comment at: clang/include/clang/Sema/SemaConcept.h:25
+  const Expr *ConstraintExpr;
+  Optional<SmallVector<TemplateArgumentLoc, 3>> ParameterMapping;
+
----------------
rsmith wrote:
> The `SmallVector` here is probably costing us space instead of saving space; we're probably better off directly allocating the correct number of parameter mappings rather than over-allocating in the (probably very common) case of one or two parameter mappings. (Plus `SmallVector` has size overhead to support resizing and capacity>size, which we should never need.)
This will leak, since we're never going to actually run the destructor for `AtomicConstraint`. (That's not a problem for compilation, but can be a problem for use of Clang as a library.) Perhaps we could store this as an `ArrayRef` to an array allocated on the `ASTContext` allocator? Alternatively, can we compute the number of parameter mappings when we create the `AtomicConstraint` object, and tail-allocate the parameter mappings?

================
Comment at: clang/lib/Parse/ParseDecl.cpp:6061-6063
+      // If we have a requires keyword here and no grouping parens, the requires
+      // clause will be parsed later as part of the init-declarator,
+      // member-declarator or function-definition.
----------------
I would just drop this comment; I don't think it's all that useful, since it's just really describing how the grammar works.

================
Comment at: clang/lib/Parse/ParseExpr.cpp:280
+           << E.get()->getSourceRange();
+      return true;
+    }
----------------
Please return false here (unless `E.isInvalid()`): we should recovering as described by the fixit hints, so we shouldn't bail out of outer parsing steps. (I suppose you should also set `NotPrimaryExpression` back to `false` for subsequent parses, to handle things like `template<typename T> requires a == b && c (X(T));`, for which we want to diagnose the missing parens around `a == b` but not assume that the `c (X(T))` is a function call.)

================
Comment at: clang/lib/Parse/ParseExpr.cpp:283
+    bool PossibleNonPrimary;
+    if (!Actions.CheckConstraintExpression(E.get(), Tok, &PossibleNonPrimary)) {
+      // Atomic constraint might be an unparenthesized non-primary expression
----------------
I think we can catch and diagnose even more cases here. If the next token is a binary operator with higher precedence than `&&` or a trailing postfix operator other than `(` (that is,  `.`, `->`, `++`, `--`, or a `[` not followed by `[`), that's always an error and we can parse the remaining tokens as a non-primary expression to recover. (For a trailing requires clause, the only thing that can come next is a function body, and that never starts with any of those tokens -- it can start with `=`, but that has lower precedence than `&&`. For a non-trailing requires clause, none of those tokens can appear next -- `(` can, and `CheckConstraintExpression` checks for that case.)

================
Comment at: clang/lib/Parse/ParseExpr.cpp:295-299
+  ExprResult LHS(ParseCastExpression(PrimaryExprOnly,
+                                     /*isAddressOfOperand=*/false,
+                                     /*isTypeCast=*/NotTypeCast,
+                                     /*isVectorLiteral=*/false,
+                                     &NotPrimaryExpression));
----------------
It'd be nice to move the call to `ParseCastExpression` into `CheckExpr` rather than duplicating it.

================
Comment at: clang/lib/Parse/ParseExpr.cpp:363-381
+  if (Tok.is(tok::l_paren) &&
+      (IsTrailingRequiresClause ||
+       (RightMostExpr->isTypeDependent() &&
+        Actions.IsDependentFunctionNameExpr(RightMostExpr)))) {
+    // We're facing one of the following cases:
+    // template<typename> requires func<T>(
+    // template<typename> void foo() requires func<T>(
----------------
Do we still need this here? `CheckConstraintExpression` already sets `PossibleNonPrimary` to `true` in cases very much like this one -- could we make it handle these remaining cases too?

================
Comment at: clang/lib/Parse/ParseExpr.cpp:1647
   case tok::kw___array_extent:
     return ParseArrayTypeTrait();

----------------
These are not //primary-expression//s.

================
Comment at: clang/lib/Sema/SemaConcept.cpp:52-81
     Diag(ConstraintExpression->getExprLoc(),
          diag::err_non_bool_atomic_constraint) << Type
         << ConstraintExpression->getSourceRange();
+    if ((Type->isFunctionType() ||
+         Type->isSpecificBuiltinType(BuiltinType::Overload)) &&
+        NextToken.is(tok::l_paren)) {
+      // We have the following case:
----------------
I think it'd be better to not produce a diagnostic here in the case where you set `PossibleNonPrimary` to `true`, and leave it to the caller to do that instead. That way we can properly recover in the parser and parse the rest of the expression (and produce a diagnostic showing where the parens should be inserted).

================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3887
+  if (!ConstraintExpr.isInvalid())
+    CorrectDelayedTyposInExpr(ConstraintExpr);
+}
----------------
We need to return the result of this back to the caller; we want to use the typo-corrected constraint expression in any further processing.

================
Comment at: clang/lib/Sema/SemaOverload.cpp:6297-6298

+  Expr *RequiresClause = Function->getTrailingRequiresClause();
+  if (RequiresClause) {
+    ConstraintSatisfaction Satisfaction;
----------------
Nit: combine these into one line (`if (Expr *RequiresClause = ...`) to minimize the scope of `RequiresClause`.

================
Comment at: clang/lib/Sema/SemaOverload.cpp:6824-6825

+  Expr *RequiresClause = Method->getTrailingRequiresClause();
+  if (RequiresClause) {
+    ConstraintSatisfaction Satisfaction;
----------------
Nit: as above, combine these to a single line.

================
Comment at: clang/lib/Sema/SemaOverload.cpp:10051
+      SecondAC = std::move(JAC);
+    }
+  }
----------------
You can `break` after this `}`. Consider flattening / simplifying the two nested loops here into a single loop (walk the candidates and collect those ones that have associated constraints, and make sure you find exactly two).

================
Comment at: clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp:2
 // RUN: %clang_cc1 -std=c++2a -fconcepts-ts -x c++ -verify %s
-
-template<typename T> requires sizeof(T) >= 2 // expected-note{{because 'sizeof(char) >= 2' (1 >= 2) evaluated to false}}
+#if 0
+template<typename T> requires (sizeof(T) >= 2) // expected-note{{because 'sizeof(char) >= 2' (1 >= 2) evaluated to false}}
----------------
Are the `#if 0`s here intentional?

================
Comment at: clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp:61-62
+
+template<typename T, typename U> requires C2<U>
+struct I<T, U> { }; // expected-error {{class template partial specialization is not more specialized than the primary template}}
----------------
It's useful to include a positive test case (`requires C2<T> && C2<U>` maybe?) in addition to the negative test case.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43357