[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
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:
> erichkeane wrote:
> > 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.
> > 
> > 
> Let me ask some questions to get in consensus for the problem:
> 
> > Basically, when we instantiate the constraints at 'checking' time, if the function is NOT a template, we call CheckFunctionConstraints.
> 
> I think the function who contains a trailing require clause should be template one. Do you want to say independent ? Or do you refer the following case?
> 
> ```C++
> template<typename T> struct A {
>   static void f(int) requires xxxx;
> };
> ```
> 
> And the related question would be what's the cases that `CheckFunctionConstraints` would be called and what's the cases that `CheckinstantiatedFunctionTemplateConstraints` would be called?
> 
> > 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).
> 
> If it is fully instantiated, how could it be dependent?
> 
> > HOWEVER, it seems we ALWAYS pick constraints off the primary template, rather than the 'stored' version of the constraint on the current declaration.
> 
> 1. What is `we`? I mean what's the function would pick up the constraints from the primary template.
> 2. Does `the stored version of the constraint` means the trailing require clause of FD? Would it be the original one or `Converted[0]`.
>>Or do you refer the following case?

Yeah, thats exactly the case I am talking about.  A case where the function itself isn't a template, but is dependent. 

>>And the related question would be what's the cases that CheckFunctionConstraints would be called and what's the cases that CheckinstantiatedFunctionTemplateConstraints would be called?

The former when either the function is not dependent at all, OR it is dependent-but-fully-instantiated (like in the example you gave).  All other cases (where the template is NOT fully instantiated) calls the other function.

>>If it is fully instantiated, how could it be dependent?

By "Fully Instantiated" I mean "everything except the constraints (and technically, some noexcept)", since we are deferring constraint checking until that point. A bunch of the changes in this commit STOP us from instantiating the constraint except when checking, since that is the point of the patch!  SO, the constriant is still 'dependent' (like in your example above).  


>>What is we? I mean what's the function would pick up the constraints from the primary template.

Royal 'we', or clang. The function is `getAssociatedConstraints`.

>>Does the stored version of the constraint means the trailing require clause of FD? Would it be the original one or Converted[0].

The 'stored' version (that is, not on the primary template) is the one that we partially instantiated when going through earlier instantiations.  In the case that it was a template, we seem to always ignore these instantiated versions.  IN the case where it is NOT a template, we use that for checking (which is why Converted[0] needs replacing here).



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

https://reviews.llvm.org/D119544