[clang] dc4259d - [c++20] Further extend the set of comparisons broken by C++20 that we
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 20 14:23:09 PDT 2020
Author: Richard Smith
Date: 2020-03-20T14:22:48-07:00
New Revision: dc4259d5a38409e65b60266a7df0f03c3b91a151
URL: https://github.com/llvm/llvm-project/commit/dc4259d5a38409e65b60266a7df0f03c3b91a151
DIFF: https://github.com/llvm/llvm-project/commit/dc4259d5a38409e65b60266a7df0f03c3b91a151.diff
LOG: [c++20] Further extend the set of comparisons broken by C++20 that we
accept as an extension.
This attempts to accept the same cases a GCC, plus cases where a
comparison is rewritten to an operator== with an integral but non-bool
return type; this is sufficient to avoid most problems with various
major open-source projects (such as ICU) and appears to fix all but one
of the comparison-related C++20 build breaks in LLVM.
This approach is being pursued for standardization.
Added:
Modified:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Overload.h
clang/lib/Sema/SemaOverload.cpp
clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p9-2a.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 90845ecf5468..ca9333d50e42 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4214,11 +4214,16 @@ def err_ovl_ambiguous_oper_binary : Error<
"use of overloaded operator '%0' is ambiguous (with operand types %1 and %2)">;
def ext_ovl_ambiguous_oper_binary_reversed : ExtWarn<
"ISO C++20 considers use of overloaded operator '%0' (with operand types %1 "
- "and %2) to be ambiguous despite there being a unique best viable function">,
+ "and %2) to be ambiguous despite there being a unique best viable function"
+ "%select{ with non-reversed arguments|}3">,
InGroup<DiagGroup<"ambiguous-reversed-operator">>, SFINAEFailure;
-def note_ovl_ambiguous_oper_binary_reversed_candidate : Note<
+def note_ovl_ambiguous_oper_binary_reversed_self : Note<
"ambiguity is between a regular call to this operator and a call with the "
"argument order reversed">;
+def note_ovl_ambiguous_oper_binary_selected_candidate : Note<
+ "candidate function with non-reversed arguments">;
+def note_ovl_ambiguous_oper_binary_reversed_candidate : Note<
+ "ambiguous candidate function with reversed arguments">;
def err_ovl_no_viable_oper : Error<"no viable overloaded '%0'">;
def note_assign_lhs_incomplete : Note<"type %0 is incomplete">;
def err_ovl_deleted_oper : Error<
@@ -4232,6 +4237,10 @@ def err_ovl_deleted_comparison : Error<
def err_ovl_rewrite_equalequal_not_bool : Error<
"return type %0 of selected 'operator==' function for rewritten "
"'%1' comparison is not 'bool'">;
+def ext_ovl_rewrite_equalequal_not_bool : ExtWarn<
+ "ISO C++20 requires return type of selected 'operator==' function for "
+ "rewritten '%1' comparison to be 'bool', not %0">,
+ InGroup<DiagGroup<"rewrite-not-bool">>, SFINAEFailure;
def err_ovl_no_viable_subscript :
Error<"no viable overloaded operator[] for type %0">;
def err_ovl_no_oper :
diff --git a/clang/include/clang/Sema/Overload.h b/clang/include/clang/Sema/Overload.h
index 6944b0b5756e..5023525aa41b 100644
--- a/clang/include/clang/Sema/Overload.h
+++ b/clang/include/clang/Sema/Overload.h
@@ -983,6 +983,14 @@ class Sema;
return CRK;
}
+ /// Determines whether this operator could be implemented by a function
+ /// with reversed parameter order.
+ bool isReversible() {
+ return AllowRewrittenCandidates && OriginalOperator &&
+ (getRewrittenOverloadedOperator(OriginalOperator) != OO_None ||
+ shouldAddReversed(OriginalOperator));
+ }
+
/// Determine whether we should consider looking for and adding reversed
/// candidates for operator Op.
bool shouldAddReversed(OverloadedOperatorKind Op);
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index dd7dbf87b947..b048c5a8d8cf 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -9420,6 +9420,49 @@ static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,
llvm_unreachable("No way to get here unless both had cpu_dispatch");
}
+/// Compute the type of the implicit object parameter for the given function,
+/// if any. Returns None if there is no implicit object parameter, and a null
+/// QualType if there is a 'matches anything' implicit object parameter.
+static Optional<QualType> getImplicitObjectParamType(ASTContext &Context,
+ const FunctionDecl *F) {
+ if (!isa<CXXMethodDecl>(F) || isa<CXXConstructorDecl>(F))
+ return llvm::None;
+
+ auto *M = cast<CXXMethodDecl>(F);
+ // Static member functions' object parameters match all types.
+ if (M->isStatic())
+ return QualType();
+
+ QualType T = M->getThisObjectType();
+ if (M->getRefQualifier() == RQ_RValue)
+ return Context.getRValueReferenceType(T);
+ return Context.getLValueReferenceType(T);
+}
+
+static bool haveSameParameterTypes(ASTContext &Context, const FunctionDecl *F1,
+ const FunctionDecl *F2, unsigned NumParams) {
+ if (declaresSameEntity(F1, F2))
+ return true;
+
+ auto NextParam = [&](const FunctionDecl *F, unsigned &I, bool First) {
+ if (First) {
+ if (Optional<QualType> T = getImplicitObjectParamType(Context, F))
+ return *T;
+ }
+ assert(I < F->getNumParams());
+ return F->getParamDecl(I++)->getType();
+ };
+
+ unsigned I1 = 0, I2 = 0;
+ for (unsigned I = 0; I != NumParams; ++I) {
+ QualType T1 = NextParam(F1, I1, I == 0);
+ QualType T2 = NextParam(F2, I2, I == 0);
+ if (!T1.isNull() && !T1.isNull() && !Context.hasSameUnqualifiedType(T1, T2))
+ return false;
+ }
+ return true;
+}
+
/// isBetterOverloadCandidate - Determines whether the first overload
/// candidate is a better candidate than the second (C++ 13.3.3p1).
bool clang::isBetterOverloadCandidate(
@@ -9487,18 +9530,20 @@ bool clang::isBetterOverloadCandidate(
break;
case ImplicitConversionSequence::Worse:
- if (Cand1.Function && Cand1.Function == Cand2.Function &&
- Cand2.isReversed()) {
+ if (Cand1.Function && Cand2.Function &&
+ Cand1.isReversed() != Cand2.isReversed() &&
+ haveSameParameterTypes(S.Context, Cand1.Function, Cand2.Function,
+ NumArgs)) {
// Work around large-scale breakage caused by considering reversed
// forms of operator== in C++20:
//
- // When comparing a function against its reversed form, if we have a
- // better conversion for one argument and a worse conversion for the
- // other, we prefer the non-reversed form.
+ // 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 conversion function from being considered ambiguous
- // with its own reversed form in various where it's only incidentally
- // heterogeneous.
+ // This prevents a comparison function from being considered ambiguous
+ // with a reversed form that is written in the same way.
//
// We diagnose this as an extension from CreateOverloadedBinOp.
HasWorseConversion = true;
@@ -9516,10 +9561,8 @@ bool clang::isBetterOverloadCandidate(
// -- for some argument j, ICSj(F1) is a better conversion sequence than
// ICSj(F2), or, if not that,
- if (HasBetterConversion)
+ if (HasBetterConversion && !HasWorseConversion)
return true;
- if (HasWorseConversion)
- return false;
// -- the context is an initialization by user-defined conversion
// (see 8.5, 13.3.1.5) and the standard conversion sequence
@@ -13256,36 +13299,56 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
// resolution for an operator@, its return type shall be cv bool
if (Best->RewriteKind && ChosenOp == OO_EqualEqual &&
!FnDecl->getReturnType()->isBooleanType()) {
- Diag(OpLoc, diag::err_ovl_rewrite_equalequal_not_bool)
+ bool IsExtension =
+ FnDecl->getReturnType()->isIntegralOrUnscopedEnumerationType();
+ Diag(OpLoc, IsExtension ? diag::ext_ovl_rewrite_equalequal_not_bool
+ : diag::err_ovl_rewrite_equalequal_not_bool)
<< FnDecl->getReturnType() << BinaryOperator::getOpcodeStr(Opc)
<< Args[0]->getSourceRange() << Args[1]->getSourceRange();
Diag(FnDecl->getLocation(), diag::note_declared_at);
- return ExprError();
+ if (!IsExtension)
+ return ExprError();
}
if (AllowRewrittenCandidates && !IsReversed &&
- CandidateSet.getRewriteInfo().shouldAddReversed(ChosenOp)) {
- // We could have reversed this operator, but didn't. Check if the
+ CandidateSet.getRewriteInfo().isReversible()) {
+ // We could have reversed this operator, but didn't. Check if some
// reversed form was a viable candidate, and if so, if it had a
// better conversion for either parameter. If so, this call is
// formally ambiguous, and allowing it is an extension.
+ llvm::SmallVector<FunctionDecl*, 4> AmbiguousWith;
for (OverloadCandidate &Cand : CandidateSet) {
- if (Cand.Viable && Cand.Function == FnDecl &&
- Cand.isReversed()) {
+ if (Cand.Viable && Cand.Function && Cand.isReversed() &&
+ haveSameParameterTypes(Context, Cand.Function, FnDecl, 2)) {
for (unsigned ArgIdx = 0; ArgIdx < 2; ++ArgIdx) {
if (CompareImplicitConversionSequences(
*this, OpLoc, Cand.Conversions[ArgIdx],
Best->Conversions[ArgIdx]) ==
ImplicitConversionSequence::Better) {
- Diag(OpLoc, diag::ext_ovl_ambiguous_oper_binary_reversed)
- << BinaryOperator::getOpcodeStr(Opc)
- << Args[0]->getType() << Args[1]->getType()
- << Args[0]->getSourceRange() << Args[1]->getSourceRange();
- Diag(FnDecl->getLocation(),
- diag::note_ovl_ambiguous_oper_binary_reversed_candidate);
+ AmbiguousWith.push_back(Cand.Function);
+ break;
}
}
- break;
+ }
+ }
+
+ if (!AmbiguousWith.empty()) {
+ bool AmbiguousWithSelf =
+ AmbiguousWith.size() == 1 &&
+ declaresSameEntity(AmbiguousWith.front(), FnDecl);
+ Diag(OpLoc, diag::ext_ovl_ambiguous_oper_binary_reversed)
+ << BinaryOperator::getOpcodeStr(Opc)
+ << Args[0]->getType() << Args[1]->getType() << AmbiguousWithSelf
+ << Args[0]->getSourceRange() << Args[1]->getSourceRange();
+ if (AmbiguousWithSelf) {
+ Diag(FnDecl->getLocation(),
+ diag::note_ovl_ambiguous_oper_binary_reversed_self);
+ } else {
+ Diag(FnDecl->getLocation(),
+ diag::note_ovl_ambiguous_oper_binary_selected_candidate);
+ for (auto *F : AmbiguousWith)
+ Diag(F->getLocation(),
+ diag::note_ovl_ambiguous_oper_binary_reversed_candidate);
}
}
}
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 023e076d50a7..f572a7932772 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
@@ -144,24 +144,26 @@ namespace problem_cases {
bool cmp_base_derived = D() == D(); // expected-warning {{ambiguous}}
template<typename T> struct CRTPBase {
- bool operator==(const T&) const; // expected-note {{operator}}
+ bool operator==(const T&) const; // expected-note {{operator}} expected-note {{reversed}}
+ bool operator!=(const T&) const; // expected-note {{non-reversed}}
};
struct CRTP : CRTPBase<CRTP> {};
- bool cmp_crtp = CRTP() == CRTP(); // expected-warning {{ambiguous}}
+ bool cmp_crtp = CRTP() == CRTP(); // expected-warning-re {{ambiguous despite there being a unique best viable function{{$}}}}}}
+ bool cmp_crtp2 = CRTP() != CRTP(); // expected-warning {{ambiguous despite there being a unique best viable function with non-reversed arguments}}
- // We can select a non-rewriteable operator== for a != comparison, when there
- // was a viable operator!= candidate we could have used instead.
- //
- // Rejecting this seems OK on balance.
+ // Given a choice between a rewritten and non-rewritten function with the
+ // same parameter types, where the rewritten function is reversed and each
+ // has a better conversion for one of the two arguments, prefer the
+ // non-rewritten one.
using UBool = signed char; // ICU uses this.
struct ICUBase {
virtual UBool operator==(const ICUBase&) const;
UBool operator!=(const ICUBase &arg) const { return !operator==(arg); }
};
struct ICUDerived : ICUBase {
- UBool operator==(const ICUBase&) const override; // expected-note {{declared here}}
+ UBool operator==(const ICUBase&) const override; // expected-note {{declared here}} expected-note {{ambiguity is between}}
};
- bool cmp_icu = ICUDerived() != ICUDerived(); // expected-error {{not 'bool'}}
+ bool cmp_icu = ICUDerived() != ICUDerived(); // expected-warning {{ambiguous}} expected-warning {{'bool', not 'problem_cases::UBool'}}
}
#else // NO_ERRORS
diff --git a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p9-2a.cpp b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p9-2a.cpp
index fce46816cedc..3826a4127910 100644
--- a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p9-2a.cpp
+++ b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p9-2a.cpp
@@ -4,18 +4,28 @@
namespace not_bool {
struct X {} x;
struct Y {} y;
- int operator==(X, Y); // expected-note 4{{here}}
+ double operator==(X, Y); // expected-note 4{{here}}
bool a = x == y; // ok
- bool b = y == x; // expected-error {{return type 'int' of selected 'operator==' function for rewritten '==' comparison is not 'bool'}}
- bool c = x != y; // expected-error {{return type 'int' of selected 'operator==' function for rewritten '!=' comparison is not 'bool'}}
- bool d = y != x; // expected-error {{return type 'int' of selected 'operator==' function for rewritten '!=' comparison is not 'bool'}}
+ bool b = y == x; // expected-error {{return type 'double' of selected 'operator==' function for rewritten '==' comparison is not 'bool'}}
+ bool c = x != y; // expected-error {{return type 'double' of selected 'operator==' function for rewritten '!=' comparison is not 'bool'}}
+ bool d = y != x; // expected-error {{return type 'double' of selected 'operator==' function for rewritten '!=' comparison is not 'bool'}}
// cv-qualifiers are OK
const bool operator==(Y, X);
bool e = y != x; // ok
// We don't prefer a function with bool return type over one witn non-bool return type.
- bool f = x != y; // expected-error {{return type 'int' of selected 'operator==' function for rewritten '!=' comparison is not 'bool'}}
+ bool f = x != y; // expected-error {{return type 'double' of selected 'operator==' function for rewritten '!=' comparison is not 'bool'}}
+
+ // As an extension, we permit integral and unscoped enumeration types too.
+ // These are used by popular C++ libraries such as ICU.
+ struct Z {} z;
+ int operator==(X, Z); // expected-note {{here}}
+ bool g = z == x; // expected-warning {{ISO C++20 requires return type of selected 'operator==' function for rewritten '==' comparison to be 'bool', not 'int'}}
+
+ enum E {};
+ E operator==(Y, Z); // expected-note {{here}}
+ bool h = z == y; // expected-warning {{ISO C++20 requires return type of selected 'operator==' function for rewritten '==' comparison to be 'bool', not 'not_bool::E'}}
}
struct X { bool equal; };
More information about the cfe-commits
mailing list