[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

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