https://github.com/usx95 created https://github.com/llvm/llvm-project/pull/72213

None

>From bc9e06ea87dca046227faf2996eb8de521863d57 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usaxen...@gmail.com>
Date: Fri, 20 Oct 2023 14:40:25 +0200
Subject: [PATCH 1/2] [clang] Handle templated operators with reversed
 arguments (#69595)

https://github.com/llvm/llvm-project/pull/68999 correctly computed
conversion sequence for reversed args to a template operators. This was
a breaking change as code, previously accepted in C++17, starts to break
in C++20.

Example:
```cpp
struct P {};
template<class S> bool operator==(const P&, const S &);

struct A : public P {};
struct B : public P {};
bool check(A a, B b) { return a == b; }  // This is now ambiguous in C++20.
```

In order to minimise widespread breakages, as a clang extension, we had
previously accepted such ambiguities with a warning
(`-Wambiguous-reversed-operator`) for non-template operators. Due to the
same reasons, we extend this relaxation for template operators.

Fixes https://github.com/llvm/llvm-project/issues/53954
---
 clang/docs/ReleaseNotes.rst                   | 21 +++++++
 clang/lib/Sema/SemaOverload.cpp               | 28 ++++++---
 .../over.match.oper/p3-2a.cpp                 | 61 +++++++++++++++++++
 3 files changed, 100 insertions(+), 10 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7a131cb520aa600..5c974eec9ed67d1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -37,6 +37,27 @@ These changes are ones which we think may surprise users 
when upgrading to
 Clang |release| because of the opportunity they pose for disruption to existing
 code bases.
 
+- Fix a bug in reversed argument for templated operators.
+  This breaks code in C++20 which was previously accepted in C++17. Eg:
+
+  .. code-block:: cpp
+
+    struct P {};
+    template<class S> bool operator==(const P&, const S&);
+
+    struct A : public P {};
+    struct B : public P {};
+
+    // This equality is now ambiguous in C++20.
+    bool ambiguous(A a, B b) { return a == b; }
+
+    template<class S> bool operator!=(const P&, const S&);
+    // Ok. Found a matching operator!=.
+    bool fine(A a, B b) { return a == b; }
+
+  To reduce such widespread breakages, as an extension, Clang accepts this code
+  with an existing warning ``-Wambiguous-reversed-operator`` warning.
+  Fixes `GH <https://github.com/llvm/llvm-project/issues/53954>`_.
 
 C/C++ Language Potentially Breaking Changes
 -------------------------------------------
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index f97e244120612e3..73f417cc8bd4024 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -7685,7 +7685,7 @@ bool Sema::CheckNonDependentConversions(
     QualType ParamType = ParamTypes[I + Offset];
     if (!ParamType->isDependentType()) {
       unsigned ConvIdx = PO == OverloadCandidateParamOrder::Reversed
-                             ? 0
+                             ? Args.size() - 1 - (ThisConversions + I)
                              : (ThisConversions + I);
       Conversions[ConvIdx]
         = TryCopyInitialization(*this, Args[I], ParamType,
@@ -10082,11 +10082,19 @@ getImplicitObjectParamType(ASTContext &Context, const 
FunctionDecl *F) {
   return M->getFunctionObjectParameterReferenceType();
 }
 
-static bool haveSameParameterTypes(ASTContext &Context, const FunctionDecl *F1,
-                                   const FunctionDecl *F2) {
+// As a Clang extension, allow ambiguity among F1 and F2 if they represent
+// represent the same entity.
+static bool allowAmbiguity(ASTContext &Context, const FunctionDecl *F1,
+                           const FunctionDecl *F2) {
   if (declaresSameEntity(F1, F2))
     return true;
-
+  if (F1->isTemplateInstantiation() && F2->isTemplateInstantiation() &&
+      declaresSameEntity(F1->getPrimaryTemplate(), F2->getPrimaryTemplate())) {
+    return true;
+  }
+  // TODO: It is not clear whether comparing parameters is necessary (i.e.
+  // different functions with same params). Consider removing this (as no test
+  // fail w/o it).
   auto NextParam = [&](const FunctionDecl *F, unsigned &I, bool First) {
     if (First) {
       if (std::optional<QualType> T = getImplicitObjectParamType(Context, F))
@@ -10271,14 +10279,14 @@ bool clang::isBetterOverloadCandidate(
     case ImplicitConversionSequence::Worse:
       if (Cand1.Function && Cand2.Function &&
           Cand1.isReversed() != Cand2.isReversed() &&
-          haveSameParameterTypes(S.Context, Cand1.Function, Cand2.Function)) {
+          allowAmbiguity(S.Context, Cand1.Function, Cand2.Function)) {
         // Work around large-scale breakage caused by considering reversed
         // forms of operator== in C++20:
         //
-        // When comparing a function against a reversed function with the same
-        // parameter types, if we have a better conversion for one argument and
-        // a worse conversion for the other, the implicit conversion sequences
-        // are treated as being equally good.
+        // When comparing a function against a reversed function, if we have a
+        // better conversion for one argument and a worse conversion for the
+        // other, the implicit conversion sequences are treated as being 
equally
+        // good.
         //
         // This prevents a comparison function from being considered ambiguous
         // with a reversed form that is written in the same way.
@@ -14449,7 +14457,7 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation 
OpLoc,
           llvm::SmallVector<FunctionDecl*, 4> AmbiguousWith;
           for (OverloadCandidate &Cand : CandidateSet) {
             if (Cand.Viable && Cand.Function && Cand.isReversed() &&
-                haveSameParameterTypes(Context, Cand.Function, FnDecl)) {
+                allowAmbiguity(Context, Cand.Function, FnDecl)) {
               for (unsigned ArgIdx = 0; ArgIdx < 2; ++ArgIdx) {
                 if (CompareImplicitConversionSequences(
                         *this, OpLoc, Cand.Conversions[ArgIdx],
diff --git 
a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp 
b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
index d83a176ec07eec9..be391c26e1e8631 100644
--- a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
+++ b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
@@ -324,6 +324,67 @@ bool x = X() == X(); // expected-warning {{ambiguous}}
 }
 } // namespace P2468R2
 
+namespace GH53954{
+namespace friend_template_1 {
+struct P {
+    template <class T>
+    friend bool operator==(const P&, const T&); // expected-note {{candidate}} 
\
+                                                // expected-note {{ambiguous 
candidate function with reversed arguments}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of 
overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+
+namespace friend_template_2 {
+struct P {
+    template <class T>
+    friend bool operator==(const T&, const P&); // expected-note {{candidate}} 
\
+                                                // expected-note {{ambiguous 
candidate function with reversed arguments}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of 
overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+
+namespace member_template {
+struct P {
+  template<class S>
+  bool operator==(const S &) const; // expected-note {{candidate}} \
+                                    // expected-note {{ambiguous candidate 
function with reversed arguments}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of 
overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+
+namespace non_member_template_1 {
+struct P {};
+template<class S>
+bool operator==(const P&, const S &); // expected-note {{candidate}} \
+                                      // expected-note {{ambiguous candidate 
function with reversed arguments}}
+
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of 
overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+
+template<class S>
+bool operator!=(const P&, const S &);
+bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
+}
+}
+
+namespace non_member_template_2 {
+struct P {};
+template<class S>
+bool operator==(const S&, const P&); // expected-note {{candidate}} \
+                                     // expected-note {{ambiguous candidate 
function with reversed arguments}}
+
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of 
overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+
 namespace ADL_GH68901{
 namespace test1 {
 namespace A {

>From 79181efd0d7aef1b8396d44cdf40c0dfa4054984 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <u...@google.com>
Date: Tue, 14 Nov 2023 07:32:44 +0100
Subject: [PATCH 2/2] accept member template ambiguities with a warning

---
 clang/lib/Sema/SemaOverload.cpp                      | 10 +++++++---
 .../over.match.funcs/over.match.oper/p3-2a.cpp       | 12 ++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 73f417cc8bd4024..9d50a5ec39f817a 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -10088,9 +10088,13 @@ static bool allowAmbiguity(ASTContext &Context, const 
FunctionDecl *F1,
                            const FunctionDecl *F2) {
   if (declaresSameEntity(F1, F2))
     return true;
-  if (F1->isTemplateInstantiation() && F2->isTemplateInstantiation() &&
-      declaresSameEntity(F1->getPrimaryTemplate(), F2->getPrimaryTemplate())) {
-    return true;
+  if (F1->isTemplateInstantiation() && F2->isTemplateInstantiation()) {
+    auto PT1 = F1->getPrimaryTemplate();
+    auto PT2 = F2->getPrimaryTemplate();
+    if (declaresSameEntity(PT1, PT2) ||
+        declaresSameEntity(PT1->getInstantiatedFromMemberTemplate(),
+                           PT2->getInstantiatedFromMemberTemplate()))
+      return true;
   }
   // TODO: It is not clear whether comparing parameters is necessary (i.e.
   // different functions with same params). Consider removing this (as no test
diff --git 
a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp 
b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
index be391c26e1e8631..486172e5d10232b 100644
--- a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
+++ b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
@@ -385,6 +385,18 @@ struct B : public P {};
 bool check(A a, B b) { return a == b; } // expected-warning {{use of 
overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
 }
 
+namespace class_and_member_template {
+template <class T>
+struct S {
+    template <typename OtherT>
+    bool operator==(const OtherT &rhs); // expected-note {{candidate}} \
+                                        // expected-note {{reversed arguments}}
+};
+struct A : S<int> {};
+struct B : S<bool> {};
+bool x = A{} == B{}; // expected-warning {{ambiguous}}
+}
+
 namespace ADL_GH68901{
 namespace test1 {
 namespace A {

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to