[clang] [clang] Reapply Handle templated operators with reversed arguments (PR #72213)
Utkarsh Saxena via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 12 05:54:55 PST 2024
https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/72213
>From b0183f23ffad814080e82c725ee4cb7902aea23f Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Fri, 12 Jan 2024 13:38:15 +0000
Subject: [PATCH 1/4] rebase
---
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 4aba054e252af2..8453b72b8b5d12 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>`_.
- The CMake variable ``GCC_INSTALL_PREFIX`` (which sets the default
``--gcc-toolchain=``) is deprecated and will be removed. Specify
``--gcc-install-dir=`` or ``--gcc-triple=`` in a `configuration file
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 64bc3851980272..a440a3b7b6a9e0 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -7724,7 +7724,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,
@@ -10121,11 +10121,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))
@@ -10329,14 +10337,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.
@@ -14526,7 +14534,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 d83a176ec07eec..be391c26e1e863 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 ca39618ec2f9e8be4cd05f56bc84131ad6a6b810 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Fri, 12 Jan 2024 13:43:16 +0000
Subject: [PATCH 2/4] recover commits
---
clang/lib/Sema/SemaOverload.cpp | 26 +++++--
.../over.match.oper/p3-2a.cpp | 72 +++++++++++++++----
2 files changed, 79 insertions(+), 19 deletions(-)
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index a440a3b7b6a9e0..23b9bc0fe2d6e2 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -7723,9 +7723,19 @@ bool Sema::CheckNonDependentConversions(
++I) {
QualType ParamType = ParamTypes[I + Offset];
if (!ParamType->isDependentType()) {
- unsigned ConvIdx = PO == OverloadCandidateParamOrder::Reversed
- ? Args.size() - 1 - (ThisConversions + I)
- : (ThisConversions + I);
+ unsigned ConvIdx;
+ if (PO == OverloadCandidateParamOrder::Reversed) {
+ ConvIdx = Args.size() - 1 - I;
+ assert(Args.size() + ThisConversions == 2 &&
+ "number of args (including 'this') must be exactly 2 for "
+ "reversed order");
+ // For members, there would be only one arg 'Args[0]' whose ConvIdx
+ // would also be 0. 'this' got ConvIdx = 1 previously.
+ assert(!HasThisConversion || (ConvIdx == 0 && I == 0));
+ } else {
+ // For members, 'this' got ConvIdx = 0 previously.
+ ConvIdx = ThisConversions + I;
+ }
Conversions[ConvIdx]
= TryCopyInitialization(*this, Args[I], ParamType,
SuppressUserConversions,
@@ -10127,9 +10137,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;
+ auto PT1 = F1->getPrimaryTemplate();
+ auto PT2 = F2->getPrimaryTemplate();
+ if (PT1 && PT2) {
+ 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 be391c26e1e863..2e374b3c62f3ce 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
@@ -327,9 +327,9 @@ bool x = X() == X(); // expected-warning {{ambiguous}}
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}}
+ template <class T>
+ friend bool operator==(const P&, const T&) { return true; } // expected-note {{candidate}} \
+ // expected-note {{ambiguous candidate function with reversed arguments}}
};
struct A : public P {};
struct B : public P {};
@@ -338,16 +338,42 @@ bool check(A a, B b) { return a == b; } // expected-warning {{use of overloaded
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}}
+ template <class T>
+ friend bool operator==(const T&, const P&) { return true; } // 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 {
+namespace friend_template_class_template {
+template<class S>
+struct P {
+ template <class T>
+ friend bool operator==(const T&, const P&) { // expected-note 2 {{candidate}}
+ return true;
+ }
+};
+struct A : public P<int> {};
+struct B : public P<bool> {};
+bool check(A a, B b) { return a == b; } // expected-warning {{ambiguous}}
+}
+
+namespace friend_template_fixme {
+// FIXME(GH70210): This should not rewrite operator== and definitely not a hard error.
+struct P {
+ template <class T>
+ friend bool operator==(const T &, const P &) { return true; } // expected-note 2 {{candidate}}
+ template <class T>
+ friend bool operator!=(const T &, const P &) { return true; } // expected-note {{candidate}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a != b; } // expected-error{{ambiguous}}
+}
+
+namespace member_template_1 {
struct P {
template<class S>
bool operator==(const S &) const; // expected-note {{candidate}} \
@@ -356,7 +382,17 @@ struct P {
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
+
+namespace member_template_2{
+template <typename T>
+class Foo {
+ public:
+ template <typename U = T>
+ bool operator==(const Foo& other) const;
+};
+bool x = Foo<int>{} == Foo<int>{};
+} // namespace template_member_opeqeq
namespace non_member_template_1 {
struct P {};
@@ -368,11 +404,9 @@ 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 &);
+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_1
namespace non_member_template_2 {
struct P {};
@@ -383,8 +417,20 @@ bool operator==(const S&, const P&); // expected-note {{candidate}} \
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_2
+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 class_and_member_template
+} // namespace
namespace ADL_GH68901{
namespace test1 {
namespace A {
>From 570cc687e79b02a1fe3ad3f9e02eb90752f3bee6 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Fri, 12 Jan 2024 13:50:40 +0000
Subject: [PATCH 3/4] add test
---
.../over.match.funcs/over.match.oper/p3-2a.cpp | 11 +++++++++++
1 file changed, 11 insertions(+)
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 2e374b3c62f3ce..d757d28860a40e 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
@@ -430,6 +430,17 @@ struct A : S<int> {};
struct B : S<bool> {};
bool x = A{} == B{}; // expected-warning {{ambiguous}}
} // namespace class_and_member_template
+
+namespace ambiguous_case {
+template <class T>
+struct Foo {};
+template <class T, class U> bool operator==(Foo<U>, Foo<T*>); // expected-note{{candidate}}
+template <class T, class U> bool operator==(Foo<T*>, Foo<U>); // expected-note{{candidate}}
+
+void test() {
+ Foo<int*>() == Foo<int*>(); // expected-error{{ambiguous}}
+}
+} // namespace ambiguous_case
} // namespace
namespace ADL_GH68901{
namespace test1 {
>From e15df5ee431b9cb779e77e81ef2576f80e49a325 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Fri, 12 Jan 2024 13:54:45 +0000
Subject: [PATCH 4/4] update release notes
---
clang/docs/ReleaseNotes.rst | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8453b72b8b5d12..3379760c5b5728 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -38,7 +38,8 @@ 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:
+ This breaks code in C++20 which was previously accepted in C++17.
+ Clang did not properly diagnose such casese in C++20 before this change. Eg:
.. code-block:: cpp
@@ -58,6 +59,7 @@ code bases.
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>`_.
+
- The CMake variable ``GCC_INSTALL_PREFIX`` (which sets the default
``--gcc-toolchain=``) is deprecated and will be removed. Specify
``--gcc-install-dir=`` or ``--gcc-triple=`` in a `configuration file
More information about the cfe-commits
mailing list