[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I've read the whole patch. It looks good to me roughly except some style issues and the 2 places you marked as ` Attn Reviewers`. Let's try to fix them one by one.



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4705
       CheckConstraintSatisfaction(
-          NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+          NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
           SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
----------------
I would feel better if we could write:
```
CheckConstraintSatisfaction(
          NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
          SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
                      TemplateArgs->getRAngleLoc()),
          Satisfaction)
```

But it looks unimplementable.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:5564
       return true;
 
     TemplateArgumentList TemplateArgs(TemplateArgumentList::OnStack, Converted);
----------------



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7468-7483
+      // Attn Reviewers: This works and fixes the constraint comparison issues,
+      // but I don't have a good idea why this is, nor if it is the 'right'
+      // thing.  I THINK it is pulling off the 'template template' level of the
+      // constraint, but I have no idea if that is the correct thing to do.
+      SmallVector<const Expr *, 3> ConvertedParamsAC;
+      TemplateArgumentList Empty(TemplateArgumentList::OnStack, {});
+      MultiLevelTemplateArgumentList MLTAL{Empty};
----------------
I've spent some time to playing it myself to figure it out. And I found that we could fix this cleaner we adopt above suggestions. The problem here is that the instantiation is started half way. But the conversion for the constraint have been deferred. So here is the problem. I guess there are other similar problems. But let's fix them after we met them actually.



================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:60-61
     const NamedDecl *D, const TemplateArgumentList *Innermost,
-    bool RelativeToPrimary, const FunctionDecl *Pattern) {
+    bool RelativeToPrimary, const FunctionDecl *Pattern, bool LookBeyondLambda,
+    bool IncludeContainingStructArgs) {
   // Accumulate the set of template argument lists in this structure.
----------------
erichkeane wrote:
> ChuanqiXu wrote:
> > Would you elaborate more for `LookBeyondLambda` and `IncludeContainingStructArgs`? It confuses me since I couldn't find `Lambda` or `Struct` from the context of use point.
> Sure!  
> 
> So this function is typically used for 'getting the template instantiation args' of the current Declaration (D) for a variety of reasons.  In all of those cases previously, it would 'stop' looking when it hit a lambda generic call operator (see line 157).  This would block our ability to get the full instantiation tree.
> 
> Similarly, it would stop at a containing class-template (as most instantiations are done against the parent class template).  Unfortunately this is sufficient, so the IncludeContainingStructArgs (see line 185) ALSO includes those arguments, as they are necessary  (since they haven't been instantiated in the constraint yet).
I got it. It might be necessary to edit the comment too.


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

https://reviews.llvm.org/D119544