ChuanqiXu added a comment. 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. In D119544#3524844 <https://reviews.llvm.org/D119544#3524844>, @erichkeane wrote: > @rsmith pointed out https://eel.is/c++draft/temp#friend-9 which I think is > supposed to fix the friend function issues: > >> A non-template friend declaration with a requires-clause shall be a >> definition. >> A friend function template with a constraint that depends on a template >> parameter from an enclosing template shall be a definition. >> Such a constrained friend function or function template declaration does not >> declare the same function or function template as a declaration in any other >> scope. > > To me, this says that > > template<typename T> > struct S { > friend void foo() requires true {} > }; > void bar() { > S<int> s; > S<float> s2; > } > > Should be perfectly fine, and NOT a redefinition? At least this: > https://godbolt.org/z/PaK1Yhn1E seems to show that all 3 compilers get this > wrong currently? Or I'm misreading this. 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. > It ALSO means that: > > template<typename T> > struct S { > template<typename U> > friend void foo() requires constriant<T> {} > }; > void bar() { > S<int> s; > S<float> s2; > } > > is perfectly fine, and not a redefinition. (CURRENT Clang and GCC get this > right, but MSVC seems to get it wrong: https://godbolt.org/z/a7dYezoPW) > > HOWEVER, By reading the rule, a slightly DIFFERENT version of that > > template<typename T> > struct S { > template<typename U> > friend void foo() requires constriant<U> {} // No longer relies on the > enclosing template > }; > void bar() { > S<int> s; > S<float> s2; > } > > Is a redefinition. All 3 compilers diagnose this. > https://godbolt.org/z/7qbYsb635 I agree with the two judgement here. It looks like the implementation of clang is right on these two versions here. ================ Comment at: clang/lib/Sema/SemaConcept.cpp:169 // operand is satisfied. - return false; + return LHSRes.isUsable() ? BO.recreateBinOp(S, LHSRes) : ExprEmpty(); ---------------- ================ Comment at: clang/lib/Sema/SemaConcept.cpp:178 // the second operand is satisfied. - return false; + return LHSRes.isUsable() ? BO.recreateBinOp(S, LHSRes) : ExprEmpty(); ---------------- ================ Comment at: clang/lib/Sema/SemaConcept.cpp:185-186 + + if (!LHSRes.isUsable() || !RHSRes.isUsable()) + return ExprEmpty(); + return BO.recreateBinOp(S, LHSRes, RHSRes); ---------------- ================ Comment at: clang/lib/Sema/SemaConcept.cpp:70 + assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good expressions?"); + assert(LHS.isUsable() && RHS.isUsable() && "Side not usable?"); + // We should just be able to 'normalize' these to the builtin Binary ---------------- erichkeane wrote: > ChuanqiXu wrote: > > I feel like the usage of the API could be further simplified. > Heh, the suggestion is inverted slightly (those should be `!isUsable`) which > caught me for a while :) Either way, I think it is a good suggestion. Oh, my bad. It is a typo. 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
