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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
