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; }; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits