Skip to content

Commit

Permalink
Revert [clang] Handle templated operators with reversed arguments and…
Browse files Browse the repository at this point in the history
… [STLExtras] Undo C++20 hack (#69937)

This breaks C++20 build of LLVM by clang 17 and earlier.
Next steps should be reduce error to a warning for
https://godbolt.org/z/s99bvq4sG
b100ca6 or similar should be reapplied
after the bug fix reached clang-18.
  • Loading branch information
usx95 committed Oct 23, 2023
1 parent 7030623 commit b997ff4
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 44 deletions.
21 changes: 0 additions & 21 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -37,27 +37,6 @@ 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
-------------------------------------------
Expand Down
28 changes: 10 additions & 18 deletions clang/lib/Sema/SemaOverload.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7685,7 +7685,7 @@ bool Sema::CheckNonDependentConversions(
QualType ParamType = ParamTypes[I + Offset];
if (!ParamType->isDependentType()) {
unsigned ConvIdx = PO == OverloadCandidateParamOrder::Reversed
? Args.size() - 1 - (ThisConversions + I)
? 0
: (ThisConversions + I);
Conversions[ConvIdx]
= TryCopyInitialization(*this, Args[I], ParamType,
Expand Down Expand Up @@ -10082,19 +10082,11 @@ getImplicitObjectParamType(ASTContext &Context, const FunctionDecl *F) {
return M->getFunctionObjectParameterReferenceType();
}

// 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) {
static bool haveSameParameterTypes(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))
Expand Down Expand Up @@ -10279,14 +10271,14 @@ bool clang::isBetterOverloadCandidate(
case ImplicitConversionSequence::Worse:
if (Cand1.Function && Cand2.Function &&
Cand1.isReversed() != Cand2.isReversed() &&
allowAmbiguity(S.Context, Cand1.Function, Cand2.Function)) {
haveSameParameterTypes(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, 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 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.
//
// This prevents a comparison function from being considered ambiguous
// with a reversed form that is written in the same way.
Expand Down Expand Up @@ -14457,7 +14449,7 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
llvm::SmallVector<FunctionDecl*, 4> AmbiguousWith;
for (OverloadCandidate &Cand : CandidateSet) {
if (Cand.Viable && Cand.Function && Cand.isReversed() &&
allowAmbiguity(Context, Cand.Function, FnDecl)) {
haveSameParameterTypes(Context, Cand.Function, FnDecl)) {
for (unsigned ArgIdx = 0; ArgIdx < 2; ++ArgIdx) {
if (CompareImplicitConversionSequences(
*this, OpLoc, Cand.Conversions[ArgIdx],
Expand Down
14 changes: 9 additions & 5 deletions llvm/include/llvm/ADT/STLExtras.h
Original file line number Diff line number Diff line change
Expand Up @@ -1291,11 +1291,15 @@ class indexed_accessor_range_base {
}

/// Compare this range with another.
template <typename OtherT> bool operator==(const OtherT &rhs) const {
return std::equal(begin(), end(), rhs.begin(), rhs.end());
}
template <typename OtherT> bool operator!=(const OtherT &rhs) const {
return !(*this == rhs);
template <typename OtherT>
friend bool operator==(const indexed_accessor_range_base &lhs,
const OtherT &rhs) {
return std::equal(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());
}
template <typename OtherT>
friend bool operator!=(const indexed_accessor_range_base &lhs,
const OtherT &rhs) {
return !(lhs == rhs);
}

/// Return the size of this range.
Expand Down

1 comment on commit b997ff4

@kazutakahirata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this patch, I am getting:

Failed Tests (1):
  Clang :: CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp

I see that the reverted patch, 47747da, modified clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp. Did you mean to revert that modification, too?

Would you mind looking into this? Thanks in advance!

Please sign in to comment.