Author: Utkarsh Saxena Date: 2024-01-12T15:03:01+01:00 New Revision: 460ff58f62456a1f3ccf61ec9cf9d10781bd41bb
URL: https://github.com/llvm/llvm-project/commit/460ff58f62456a1f3ccf61ec9cf9d10781bd41bb DIFF: https://github.com/llvm/llvm-project/commit/460ff58f62456a1f3ccf61ec9cf9d10781bd41bb.diff LOG: [clang] Reapply Handle templated operators with reversed arguments (#72213) Re-applies https://github.com/llvm/llvm-project/pull/69595 with extra [diff](https://github.com/llvm/llvm-project/pull/72213/commits/79181efd0d7aef1b8396d44cdf40c0dfa4054984) ### New changes Further relax ambiguities with a warning for member operators of a template class (primary templates of such ops do not match). Eg: ```cpp template <class T> struct S { template <typename OtherT> bool operator==(const OtherT &rhs); }; struct A : S<int> {}; struct B : S<bool> {}; bool x = A{} == B{}; // accepted with a warning. ``` This is important for making llvm build using previous clang versions in C++20 mode (eg: this makes the commit e558be51bab051d1471d92e967f8a2aecc13567a keep working with a warning instead of an error). ### Description from https://github.com/llvm/llvm-project/pull/69595 https://github.com/llvm/llvm-project/pull/68999 correctly computed conversion sequence for reversed args to a template operator. 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 Added: Modified: clang/docs/ReleaseNotes.rst clang/lib/Sema/SemaOverload.cpp clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 10e426a30d98f9..3cbce1be159437 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -37,6 +37,29 @@ 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. + Clang did not properly diagnose such casese in C++20 before this change. 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..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 - ? 0 - : (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, @@ -10121,11 +10131,23 @@ 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; - + 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. + // diff erent 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 +10351,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 +14548,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..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 @@ -324,6 +324,124 @@ 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&) { 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 friend_template_2 { +struct P { + 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 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}} \ + // 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 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 {}; +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_1 + +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 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 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 { namespace A { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits