[PATCH] D50360: [Concepts] Requires Expressions

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

rsmith added a comment.

(Partial comments; I'll try to complete the review today or tomorrow.)



================
Comment at: clang/include/clang/AST/ExprCXX.h:4933
 
+
+/// \brief A static requirement that can be used in a requires-expression to
----------------
You're adding a total of ~400 lines here; that seems sufficient to factor out into a separate header (`ExprConcepts.h`, containing this and `ConceptSpecializationExpr` maybe?)


================
Comment at: clang/include/clang/AST/ExprCXX.h:4936
+/// check properties of types and expression.
+class Requirement {
+public:
----------------
This name (`clang::Requirement`) is a little too general for my tastes; consider moving these to a sub-namespace.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2714-2716
+      if (RetReq.isTypeConstraint())
+        TRY_TO(TraverseTemplateParameterListHelper(
+                   RetReq.getTypeConstraintTemplateParameterList()));
----------------
We should traverse the entire //type-constraint// here, including the nested name specifier and concept name.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:766
+def err_requires_expr_simple_requirement_noexcept : Error<
+  "'noexcept' can only be used in a compound requirements (with '{' '}' around "
+  "the expression)">;
----------------
requirements -> requirement


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:774
+  "you intend to place it in a nested requirement (add another 'requires' "
+  "before the expression)">, InGroup<DiagGroup<"requires-expression">>;
 
----------------
Missing `?` somewhere in this diagnostic, I think.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2638
+  "variable template|alias template|template template parameter|template}1 "
+  "and not a type template">;
+def err_type_requirement_no_such_type : Error<
----------------
" and not" -> ", not"

would be a bit more consistent with how we phrase similar things elsewhere.


================
Comment at: clang/include/clang/Sema/Sema.h:6285
   void
-  DiagnoseUnsatisfiedConstraint(const ConstraintSatisfaction& Satisfaction);
+  DiagnoseUnsatisfiedConstraint(const ConstraintSatisfaction& Satisfaction,
+                                bool First = true);
----------------
`&` on the right, please (here and below).


================
Comment at: clang/lib/AST/ExprCXX.cpp:1848-1855
+const TypeConstraint *
+ExprRequirement::ReturnTypeRequirement::getTypeConstraint() const {
+  assert(isTypeConstraint());
+  auto TPL =
+      TypeConstraintInfo.getPointer().get<TemplateParameterList *>();
+  return cast<TemplateTypeParmDecl>(TPL->getParam(0))
+      ->getTypeConstraint();
----------------
This representation for a type-constraint seems a bit surprising. Do we really need the template parameter + template parameter list here? It seems to me like we should only really need the `TypeConstraint` itself, plus its immediately-declared constraint.


Repository:
  rC Clang

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

https://reviews.llvm.org/D50360