[PATCH] D50360: [Concepts] Requires Expressions

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

rsmith added inline comments.


================
Comment at: include/clang/AST/DeclCXX.h:2051
+/// \code
+/// template<typename T> requires requires (T t) { {t++} -> Regular };
+/// \endcode
----------------
The semicolon here is in the wrong place (should be before the final `}`, not after).


================
Comment at: include/clang/AST/DeclTemplate.h:1185
   bool wereArgumentsSpecified() const {
-    return ArgsAsWritten == nullptr;
+    return ArgsAsWritten != nullptr;
   }
----------------
Please move this bugfix earlier in the patch series.


================
Comment at: include/clang/AST/ExprCXX.h:4634
+  llvm::SmallVector<Requirement *, 3> Requirements;
+  SourceLocation RBraceLoc;
+
----------------
Reorder this next to `RequiresKWLoc`. `SourceLocation`s are 4 bytes, whereas for most platforms we care about, pointers are size-8 and align-8, so the reordering will save 8 bytes of padding.


================
Comment at: include/clang/AST/ExprCXX.h:4645
+
+  void setLocalParameters(ArrayRef<ParmVarDecl *> LocalParameters);
+  ArrayRef<ParmVarDecl *> getLocalParameters() const { return LocalParameters; }
----------------
Please avoid adding setters to `Expr` subclasses if possible. We want the AST to be immutable. (Befriend `ASTReaderStmt` instead.)


================
Comment at: include/clang/AST/ExprCXX.h:4674
+  llvm::SmallVector<ParmVarDecl *, 2> LocalParameters;
+  llvm::SmallVector<Requirement *, 3> Requirements;
+  SourceLocation RBraceLoc;
----------------
riccibruno wrote:
> Can you tail-allocate them ?
Using a `SmallVector` here is a bug; the destructor is not run on `Expr` nodes so this will leak memory. Please do change to using tail allocation here.


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:752
+def err_requires_expr_simple_requirement_unexpected_tok : Error<
+  "unexpected %0 after expression. Did you intend to use a compound "
+  "requirement? (with '{' '}' around the expression)">;
----------------
`. Did` -> `; did` so this fits the general diagnostic pattern regardless of whether the diagnostic renderer capitalizes the diagnostic.


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:753
+  "unexpected %0 after expression. Did you intend to use a compound "
+  "requirement? (with '{' '}' around the expression)">;
 
----------------
Move `?` to the end.


================
Comment at: lib/AST/ExprCXX.cpp:32-35
 #include "clang/Sema/Template.h"
 #include "clang/Sema/SemaDiagnostic.h"
 #include "clang/Sema/Sema.h"
+#include "clang/Sema/Overload.h"
----------------
These `#include`s are all unacceptable. `AST` is layered below `Sema`, and cannot depend on `Sema` headers and classes.


================
Comment at: lib/AST/ExprConstant.cpp:9953-9955
+  if (E->isValueDependent()) {
+    return Error(E);
+  }
----------------
It is a logic error for the expression evaluator to encounter a value-dependent expression, and we assert on the way into the evaluator if we would encounter one. You don't need to check this here.


================
Comment at: lib/AST/StmtProfile.cpp:1325
+      //    expression. It is equivalent to the simple-requirement x++; [...]
+      // We therefore do not profile isSimple() here.
+      ID.AddBoolean(ExprReq->getNoexceptLoc().isValid());
----------------
We don't /need to/ profile `isSimple`, but we still could. (This "is equivalent to" doesn't override the general ODR requirement that you spell the expression with the same token sequence.)

Do we mangle simple and compound requirements the same way? (Has a mangling for requires-expressions even been proposed on the Itanium ABI list yet?)


================
Comment at: lib/Parse/ParseExprCXX.cpp:3097
+    Braces.consumeClose();
+    return ExprError();
+  }
----------------
Recovering by producing an (always-satisfied) `RequiresExpr` with no requirements would seem reasonable here.


================
Comment at: lib/Parse/ParseExprCXX.cpp:3113
+    switch (Tok.getKind()) {
+    case tok::kw_typename: {
+      // Type requirement
----------------
This is incorrect. A //simple-requirement// can begin with the `typename` keyword. (Eg, `requires { typename T::type() + 1; }`)

The right way to handle this is to call `TryAnnotateTypeOrScopeToken()` when you see `tok::kw_typename`, and then detect a `type-requirement` as a `tok::annot_typename` followed by a `tok::semi`. (By following that approach, you can also handle cases where the `typename` is missing.) You'll need to deal specially with the case where the //nested-name-specifier// is omitted, since in that case the `typename` keyword does not form part of a //typename-specifier//; in that case, after `TryAnnotateTypeOrScopeToken()` you'll have a `tok::kw_typename`, `tok::identifier`, `tok::semi` sequence or a `tok::kw_typename`, `tok::annot_template_id`, `tok::semi` sequence to detect and special-case.


================
Comment at: lib/Parse/ParseExprCXX.cpp:3125
+          SS.isInvalid()) {
+        Braces.skipToEnd();
+        Actions.ActOnExitRequiresExpr();
----------------
Do we need to skip the entirety of the //requires-expression// in this case? Skipping to the semicolon would seem sufficient, and would allow us to recover better.


================
Comment at: lib/Parse/ParseExprCXX.cpp:3299
+    }
+    case tok::kw_requires: {
+      // Nested requirement
----------------
You need to distinguish between a //simple-requirement// and a //nested-requirement// here. `requires { requires { 0; }; };` is valid and has a //requires-expression// as a //simple-requirement//. (We should issue a warning on such cases, since only the validity of the //requires-expression// is checked -- and it's always valid -- and its result is ignored. But we should at least parse it properly.)

This might need unbounded lookahead. Eg, consider `requires { requires (A<T1, T2, T3> && B)`: if the next token is `{`, it's a //requires-expression// in a //simple-requirement//. Otherwise, it's a //nested-requirement//. In general, I think this is correct:

 * If the token after `requires` is `{`, we have a //requires-expression// in a //simple-requirement//.
 * If the token after `requires` is `(`, skip to the matching `)`; if the next token is `{`, we have a //requires-expression// in a //simple-requirement//.
 * In all other cases, we have a //nested-requirement//.

You can optimize the second bullet by tentatively parsing a function parameter list (`TryParseFunctionDeclarator`) rather than just skipping to the `)`; that will often let us determine that we have a //nested-requirement// by inspecting fewer tokens. (And hopefully in the common case we can just see that the parenthesized tokens begin with a template-id naming a concept, so we can't possibly have a function parameter list.)


================
Comment at: lib/Sema/SemaExpr.cpp:1809
 
+static bool isEvaluatableContext(Sema &SemaRef);
+
----------------
(Unused.)


================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:1021-1052
+    ExprResult TransformRequiresExpr(RequiresExpr *E) {
+      LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
+      return TreeTransform<TemplateInstantiator>::TransformRequiresExpr(E);
+    }
+
+    bool TransformRequiresExprRequirements(ArrayRef<Requirement *> Reqs,
+        SmallVectorImpl<Requirement *> &Transformed) {
----------------
Do we really need to duplicate the `TreeTransform` code here, and add a representation for a requirement whose substitution failed, and so on?

Possible alternative: create a `SFINAETrap` for the entire requires-expression, perform a `TreeTransform` of the whole thing, and if it fails, capture the diagnostic and build a `FailedRequiresExpr` node that stores the diagnostic and evaluates to `false`. (Under this model, a non-value-dependent `RequiresExpr` would always evaluate to `true`.) That should simplify this code, and the representations of `RequiresExpr` and `Requirements` (since you don't need to model "substitution failed" any more, nor conditionally store a diagnostic on a requirement).

Are there cases where we want to retain more information than that? (I don't think there are, since you don't actually retain any useful information from the requirement that failed, other than the diagnostic itself, but maybe I've missed something.) The results of a successful substitution into some parts of a failed `RequiresExpr` don't really seem interesting.


================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:1028
+        SmallVectorImpl<Requirement *> &Transformed) {
+      bool SatisfcationDetermined = false;
+      for (Requirement *Req : Reqs) {
----------------
Typo `Satisfcation` -> `Satisfaction`


================
Comment at: lib/Serialization/ASTWriterStmt.cpp:14
 
+#include <clang/Sema/DeclSpec.h>
 #include "clang/Serialization/ASTWriter.h"
----------------
Don't use angled includes to find Clang headers. (Also, the use of this header here is deeply suspicious: this header describes an interface between `Sema` and the `Parser` that `Serialization` should generally not need to know about.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D50360