[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3526810 <https://reviews.llvm.org/D119544#3526810>, @ChuanqiXu wrote:

> In D119544#3494281 <https://reviews.llvm.org/D119544#3494281>, @erichkeane wrote:
>
>> Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts.  I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu  a chance to check out the changes.
>>
>> This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020
>
> Sorry that I missed the ping. The change in that revision looks good to me.
>
>> I THINK what we want to do instead of trying to 're setup' the template arguments (and instead of just 'keeping' the uninstantiated expression) is to run some level of 'tree-transform' that updates the names of the template parameters (including depths/etc), but without doing lookup. This I think would fix our 'friend function' issue, and our 'comparing constraints' stuff work correctly as well (rather than storing an instantiated version, but not reusing it except for comparing them).
>
> I agree this should be a better direction.

I actually got a response from Richard who seems to be more in favor of the solution I tried initially (the one in this patch!).  The problems I have with it I think get solved by the 'friend function' rules that I pasted above, so I THINK I can fix those and be ok.  I'll still need SOME level of tree-transform, but only to see if it depends on the enclosing template.

> I agree this one should be fine. I think we could fix the problem by making non-template function inside a template class to be a dependent type. Like this one is accepted: >https://godbolt.org/z/MorzcqE1a
> This bug might not be horrible since few developer would write friend function definition which doesn't touch the enclosing class.

Yeah, that is a really strange one.  I don't think we can make it dependent as then it wouldn't be callable when fully instantiate-able.  I don't have a good idea how to make this work in the AST though, and might end up leaving it as a 'fixme'.  As you said, I think you're right that this is a very small bug-vector.


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

https://reviews.llvm.org/D119544