[PATCH] D41284: [Concepts] Associated constraints infrastructure.

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

rsmith added inline comments.


================
Comment at: lib/AST/DeclTemplate.cpp:203
+
+Expr *TemplateDecl::getAssociatedConstraints() {
+  return getOrCollectAssociatedConstraints(getASTContext(),
----------------
Rather than producing an `Expr*` (which will presumably eventually need to contain synthetic `&&` expressions), this should produce a vector of constraints (notionally and'd together), which will (eventually) be either constraint-expressions or some representation of "the constraints implied by this template parameter". If you do that, I don't think there's any reason to cache this information on the `TemplateDecl`; it should be cheap enough to recompute when we need it.


================
Comment at: lib/Sema/SemaConcept.cpp:47-58
+                                             const Expr *NewAC) {
+  if (!(NewAC || OldAC))
+    return true; // Nothing to check; no mismatch.
+  if (NewAC && OldAC) {
+    llvm::FoldingSetNodeID OldACInfo, NewACInfo;
+    NewAC->Profile(NewACInfo, Context, /*Canonical=*/true);
+    OldAC->Profile(OldACInfo, Context, /*Canonical=*/true);
----------------
I'm sure we already have this "profile two expressions and check they are canonically equivalent" functionality factored out //somewhere//. Is there nothing like that that's appropriate to reuse here?


================
Comment at: lib/Sema/SemaTemplate.cpp:2145-2154
   if (RemoveDefaultArguments) {
-    for (TemplateParameterList::iterator NewParam = NewParams->begin(),
-                                      NewParamEnd = NewParams->end();
-         NewParam != NewParamEnd; ++NewParam) {
-      if (TemplateTypeParmDecl *TTP = dyn_cast<TemplateTypeParmDecl>(*NewParam))
+    for (NamedDecl *NewParam : *NewParams) {
+      if (TemplateTypeParmDecl *TTP = dyn_cast<TemplateTypeParmDecl>(NewParam))
         TTP->removeDefaultArgument();
       else if (NonTypeTemplateParmDecl *NTTP
-                                = dyn_cast<NonTypeTemplateParmDecl>(*NewParam))
+                                = dyn_cast<NonTypeTemplateParmDecl>(NewParam))
         NTTP->removeDefaultArgument();
----------------
Unrelated cleanup: please commit this separately.


================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3141-3148
+  Expr *InstRequiresClause = nullptr;
+  if (Expr *E = L->getRequiresClause()) {
+    ExprResult Res = SemaRef.SubstExpr(E, TemplateArgs);
+    if (Res.isInvalid() || !Res.isUsable()) {
+      return nullptr;
+    }
+    InstRequiresClause = Res.get();
----------------
This is wrong (we should never substitute into a constraint-expression except as part of satisfaction or subsumption checks). Please at least add a FIXME.


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2003-2006
+    bool HasAssociatedConstraints = Record.readInt();
+    if (HasAssociatedConstraints) {
+       AssociatedConstraints = Record.readExpr();
+    }
----------------
There doesn't seem to be a good reason to serialize the cached associated constraints, even if we want to keep them (which I don't think we do).


================
Comment at: lib/Serialization/ASTWriter.cpp:5856
   for (const auto &P : *TemplateParams)
-    AddDeclRef(P);
+    AddDeclRef(P); // TODO: Concepts - constrained parameters.
+  if (const Expr *RequiresClause = TemplateParams->getRequiresClause()) {
----------------
What would be left "to do" here? For constrained parameters, there may be something interesting to track on the `*Template*ParmDecl`, but not here, I wouldn't think,


Repository:
  rC Clang

https://reviews.llvm.org/D41284