[clang] [clang] Handle templated operators with reversed arguments (PR #69595)
Utkarsh Saxena via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 20 04:24:25 PDT 2023
https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/69595
>From 4460b02508d21d98cf05103bece99fc5bd474ab0 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Thu, 19 Oct 2023 12:33:45 +0200
Subject: [PATCH 1/5] Reapply "Correctly compute conversion seq for args to fn
with reversed param order (#68999)"
This reverts commit a3a0f59a1e1cb0ac02f06b19f730ea05a6541c96.
---
clang/docs/ReleaseNotes.rst | 2 ++
clang/lib/Sema/SemaOverload.cpp | 2 +-
.../over.match.oper/p3-2a.cpp | 35 +++++++++++++++++++
3 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index eee48431d716878..f6ad453cb17f9d9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -117,6 +117,8 @@ C++ Language Changes
C++20 Feature Support
^^^^^^^^^^^^^^^^^^^^^
+- Fix a bug in conversion sequence of arguments to a function with reversed parameter order.
+ Fixes `GH <https://github.com/llvm/llvm-project/issues/53954>`_.
C++23 Feature Support
^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index ce78994e6553814..c271cebb9eb638f 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -7688,7 +7688,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,
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 5c6804eb7726b5f..02fe37dc1be5058 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,41 @@ bool x = X() == X(); // expected-warning {{ambiguous}}
}
} // namespace P2468R2
+namespace GH53954{
+namespace test1 {
+struct P {
+ template <class T>
+ friend bool operator==(const P&, const T&); // expected-note {{candidate}} \
+ // expected-note {{reversed parameter order}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-error {{ '==' is ambiguous}}
+}
+
+namespace test2 {
+struct P {
+ template <class T>
+ friend bool operator==(const T&, const P&); // expected-note {{candidate}} \
+ // expected-note {{reversed parameter order}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-error {{ '==' is ambiguous}}
+}
+
+namespace test3 {
+struct P {
+ template<class S>
+ bool operator==(const S &) const; // expected-note {{candidate}} \
+ // expected-note {{reversed parameter order}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-error {{ '==' is ambiguous}}
+}
+}
+
#else // NO_ERRORS
namespace problem_cases {
>From 12f5b5e43e51fbd966fc99d82c5d04019cfa3adf Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Thu, 19 Oct 2023 14:02:24 +0200
Subject: [PATCH 2/5] Do not hard error for ambiguity with reversed arguments
for templated operators
---
clang/docs/ReleaseNotes.rst | 18 ++++++++-
clang/lib/Sema/SemaOverload.cpp | 10 +++--
.../over.match.oper/p3-2a.cpp | 40 ++++++++++++++-----
3 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f6ad453cb17f9d9..a38cd74b6165c84 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -37,6 +37,22 @@ 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:
+ ```
+ 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 check(A a, B b) { return a == b; }
+ ```
+ To reduce widespread breakages, as an extension, clang would accept this with a
+ `-Wambiguous-reversed-operator` warning.
+ Fixes `GH <https://github.com/llvm/llvm-project/issues/53954>`_.
+
+
C/C++ Language Potentially Breaking Changes
-------------------------------------------
@@ -117,8 +133,6 @@ C++ Language Changes
C++20 Feature Support
^^^^^^^^^^^^^^^^^^^^^
-- Fix a bug in conversion sequence of arguments to a function with reversed parameter order.
- Fixes `GH <https://github.com/llvm/llvm-project/issues/53954>`_.
C++23 Feature Support
^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index c271cebb9eb638f..53de7cb783a6ced 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -10085,10 +10085,14 @@ getImplicitObjectParamType(ASTContext &Context, const FunctionDecl *F) {
return M->getFunctionObjectParameterReferenceType();
}
-static bool haveSameParameterTypes(ASTContext &Context, const FunctionDecl *F1,
+static bool allowAmbiguityWithSelf(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 NextParam = [&](const FunctionDecl *F, unsigned &I, bool First) {
if (First) {
@@ -10274,7 +10278,7 @@ bool clang::isBetterOverloadCandidate(
case ImplicitConversionSequence::Worse:
if (Cand1.Function && Cand2.Function &&
Cand1.isReversed() != Cand2.isReversed() &&
- haveSameParameterTypes(S.Context, Cand1.Function, Cand2.Function)) {
+ allowAmbiguityWithSelf(S.Context, Cand1.Function, Cand2.Function)) {
// Work around large-scale breakage caused by considering reversed
// forms of operator== in C++20:
//
@@ -14421,7 +14425,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)) {
+ allowAmbiguityWithSelf(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 02fe37dc1be5058..e075580828f42e5 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
@@ -325,38 +325,60 @@ bool x = X() == X(); // expected-warning {{ambiguous}}
} // namespace P2468R2
namespace GH53954{
-namespace test1 {
+namespace friend_template_1 {
struct P {
template <class T>
friend bool operator==(const P&, const T&); // expected-note {{candidate}} \
- // expected-note {{reversed parameter order}}
+ // 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-error {{ '==' is ambiguous}}
+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 test2 {
+namespace friend_template_2 {
struct P {
template <class T>
friend bool operator==(const T&, const P&); // expected-note {{candidate}} \
- // expected-note {{reversed parameter order}}
+ // 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-error {{ '==' is ambiguous}}
+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 test3 {
+namespace member_template {
struct P {
template<class S>
bool operator==(const S &) const; // expected-note {{candidate}} \
- // expected-note {{reversed parameter order}}
+ // 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-error {{ '==' is ambiguous}}
+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}}
+}
+}
+
+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}}
}
#else // NO_ERRORS
>From 7b7a53d59bfec63fc5bcf9e7bd5beee99c4c0358 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Thu, 19 Oct 2023 14:49:52 +0200
Subject: [PATCH 3/5] Add more test and update docs
---
clang/docs/ReleaseNotes.rst | 11 ++++++-----
clang/lib/Sema/SemaOverload.cpp | 8 ++++----
.../over.match.funcs/over.match.oper/p3-2a.cpp | 4 ++++
3 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a38cd74b6165c84..927f421c0258e16 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -41,19 +41,20 @@ code bases.
This breaks code in C++20 which was previously accepted in C++17. Eg:
```
struct P {};
- template<class S> bool operator==(const P&, const S &);
+ 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 check(A a, B b) { return a == b; }
+
+ bool ambiguous(A a, B b) { return a == b; } // This equality is now ambiguous in C++20.
+
+ template<class S> bool operator!=(const P&, const S&);
+ bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
```
To reduce widespread breakages, as an extension, clang would accept this with a
`-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 53de7cb783a6ced..00c4f4d4182eb8e 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -10282,10 +10282,10 @@ bool clang::isBetterOverloadCandidate(
// 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.
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 e075580828f42e5..5727d506fe5e61d 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
@@ -367,6 +367,10 @@ bool operator==(const P&, const S &); // 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}}
+
+template<class S>
+bool operator!=(const P&, const S &);
+bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
}
}
>From ab3a6d8fb53b7ef1eb1d8055d412317f18bde492 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Thu, 19 Oct 2023 15:01:30 +0200
Subject: [PATCH 4/5] fix release notes
---
clang/docs/ReleaseNotes.rst | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 927f421c0258e16..489ddf137a693bc 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -39,20 +39,22 @@ 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:
- ```
- struct P {};
- template<class S> bool operator==(const P&, const S&);
- struct A : public P {};
- struct B : public P {};
+ .. code-block:: cpp
+
+ struct P {};
+ template<class S> bool operator==(const P&, const S&);
+
+ struct A : public P {};
+ struct B : public P {};
+
+ bool ambiguous(A a, B b) { return a == b; } // This equality is now ambiguous in C++20.
- bool ambiguous(A a, B b) { return a == b; } // This equality is now ambiguous in C++20.
+ template<class S> bool operator!=(const P&, const S&);
+ bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
- template<class S> bool operator!=(const P&, const S&);
- bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
- ```
To reduce widespread breakages, as an extension, clang would accept this with a
- `-Wambiguous-reversed-operator` warning.
+ ``-Wambiguous-reversed-operator`` warning.
Fixes `GH <https://github.com/llvm/llvm-project/issues/53954>`_.
C/C++ Language Potentially Breaking Changes
>From 5cb02abf4727c23faaaae7ee67be25b4b555d23e Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Fri, 20 Oct 2023 13:24:09 +0200
Subject: [PATCH 5/5] address comments
---
clang/docs/ReleaseNotes.rst | 4 ++--
clang/lib/Sema/SemaOverload.cpp | 14 +++++++++-----
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 489ddf137a693bc..d3b2dc2f2b9960e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -53,8 +53,8 @@ code bases.
template<class S> bool operator!=(const P&, const S&);
bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
- To reduce widespread breakages, as an extension, clang would accept this with a
- ``-Wambiguous-reversed-operator`` warning.
+ 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 00c4f4d4182eb8e..d57a7ad8f46859a 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -10085,15 +10085,19 @@ getImplicitObjectParamType(ASTContext &Context, const FunctionDecl *F) {
return M->getFunctionObjectParameterReferenceType();
}
-static bool allowAmbiguityWithSelf(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))
@@ -10278,7 +10282,7 @@ bool clang::isBetterOverloadCandidate(
case ImplicitConversionSequence::Worse:
if (Cand1.Function && Cand2.Function &&
Cand1.isReversed() != Cand2.isReversed() &&
- allowAmbiguityWithSelf(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:
//
@@ -14425,7 +14429,7 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
llvm::SmallVector<FunctionDecl*, 4> AmbiguousWith;
for (OverloadCandidate &Cand : CandidateSet) {
if (Cand.Viable && Cand.Function && Cand.isReversed() &&
- allowAmbiguityWithSelf(Context, Cand.Function, FnDecl)) {
+ allowAmbiguity(Context, Cand.Function, FnDecl)) {
for (unsigned ArgIdx = 0; ArgIdx < 2; ++ArgIdx) {
if (CompareImplicitConversionSequences(
*this, OpLoc, Cand.Conversions[ArgIdx],
More information about the cfe-commits
mailing list