[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:7011
+
+  bool CheckConstraintSatisfaction(
+      const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
----------------
ChuanqiXu wrote:
> I think this one need comment too. What's the difference with the above one?
Thanks! I'll comment on that.  The difference is the 'out' parameter of 'ConvertedConstraints', but I'll put details in the comment.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:489
+  // Attn Reviewers: we need to do this for the function constraints for
+  // comparison of constraints to work, but do we also need to do it for
+  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we
----------------
ChuanqiXu wrote:
> Would you mind to elaborate for the issue "function constraints for comparison of constraints to work" more? Maybe it is said in previous messages, but the history is hard to follow...
Yep, this one is difficult :/  Basically, when we instantiate the constraints at 'checking' time, if the function is NOT a template, we call `CheckFunctionConstraints`. When we go to check the 'subsumption' type things with a fully instantiated version, they fail if they are dependent (as it is expected that way).  See the `setTrailingRequiresClause` here.

HOWEVER, it seems we ALWAYS pick constraints off the primary template, rather than the 'stored' version of the constraint on the current declaration.  I tried to do something similar here for those cases, but 1: it had no effect seemingly, and 2: it ended up getting complicated in many cases (as figuring out the relationship between the constraints in the two nodes was difficult/near impossible).

I opted to not do it, and I don't THINK it needs to happen over there, but I wanted to point out that I was skipping it in case reviewers had a better idea.




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

https://reviews.llvm.org/D119544