diff --git a/clang/include/clang/Sema/Overload.h b/clang/include/clang/Sema/Overload.h --- a/clang/include/clang/Sema/Overload.h +++ b/clang/include/clang/Sema/Overload.h @@ -1005,23 +1005,25 @@ CRK = OverloadCandidateRewriteKind(CRK | CRK_Reversed); 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)); - } + /// Determines whether this operator could be implemented by a function + /// with reversed parameter order. + bool isReversible() { + return RewriteInfo.AllowRewrittenCandidates && + RewriteInfo.OriginalOperator && + (getRewrittenOverloadedOperator(RewriteInfo.OriginalOperator) != + OO_None || + mayAddReversed(RewriteInfo.OriginalOperator)); + } - /// Determine whether we should consider looking for and adding reversed - /// candidates for operator Op. - bool shouldAddReversed(OverloadedOperatorKind Op); + /// Determine whether we may consider looking for and adding reversed + /// candidates for operator Op. + bool mayAddReversed(OverloadedOperatorKind Op); - /// Determine whether we should add a rewritten candidate for \p FD with - /// reversed parameter order. - bool shouldAddReversed(ASTContext &Ctx, const FunctionDecl *FD); - }; + /// Determine whether we should add a rewritten candidate for \p FD with + /// reversed parameter order. + bool shouldAddReversed(Sema &S, ArrayRef Args, const FunctionDecl *FD); private: SmallVector Candidates; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -3765,7 +3765,8 @@ ExprResult CheckConvertedConstantExpression(Expr *From, QualType T, APValue &Value, CCEKind CCE, NamedDecl *Dest = nullptr); - + // bool HasSameExclaimEqual(const FunctionDecl *FD, SourceLocation OpLoc, + // Expr *RHSArg); /// Abstract base class used to perform a contextual implicit /// conversion from an expression to any type passing a filter. class ContextualImplicitConverter { diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp --- a/clang/lib/Sema/SemaOverload.cpp +++ b/clang/lib/Sema/SemaOverload.cpp @@ -12,14 +12,17 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/CXXInheritance.h" +#include "clang/AST/DeclCXX.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/DependenceFlags.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/ExprObjC.h" +#include "clang/AST/Type.h" #include "clang/AST/TypeOrdering.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticOptions.h" +#include "clang/Basic/OperatorKinds.h" #include "clang/Basic/PartialDiagnostic.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/TargetInfo.h" @@ -34,6 +37,7 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallString.h" +#include "llvm/Support/Casting.h" #include #include @@ -884,22 +888,80 @@ } } -bool OverloadCandidateSet::OperatorRewriteInfo::shouldAddReversed( - OverloadedOperatorKind Op) { - if (!AllowRewrittenCandidates) +static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc, + ArrayRef Args, + const FunctionDecl *EqEqFD) { + assert(EqEqFD->getOverloadedOperator() == + OverloadedOperatorKind::OO_EqualEqual); + // C++2a [over.match.oper]p4: + // A non-template function or function template F named operator== is a + // rewrite target with first operand o unless a search for the name operator!= + // in the scope S from the instantiation context of the operator expression + // finds a function or function template that would correspond + // ([basic.scope.scope]) to F if its name were operator==, where S is the + // scope of the class type of o if F is a class member, and the namespace + // scope of which F is a member otherwise. A function template specialization + // named operator== is a rewrite target if its function template is a rewrite + // target. + DeclarationName NotEqOp = S.Context.DeclarationNames.getCXXOperatorName( + OverloadedOperatorKind::OO_ExclaimEqual); + if (auto *MD = dyn_cast(EqEqFD)) { + // If F is a class member, search scope is class type of first operand + // (Args[1] since Args must be reversed). + QualType RHS = Args[1]->getType(); + if (auto *RHSRec = RHS->getAs()) { + LookupResult Members(S, NotEqOp, OpLoc, + Sema::LookupNameKind::LookupOrdinaryName); + S.LookupQualifiedName(Members, RHSRec->getDecl()); + Members.suppressDiagnostics(); + for (NamedDecl *Op : Members) + if (S.Context.hasSameUnqualifiedType( + MD->getParamDecl(0)->getType(), + Op->getAsFunction()->getParamDecl(0)->getType())) + return false; + } + // FIXME: Disallow reversal of symmetric ==. + // Eg. `struct S{ bool operator==(const S&);};` + return true; + } + // Otherwise the search scope is the namespace scope of which F is a member. + LookupResult NonMembers(S, NotEqOp, OpLoc, + Sema::LookupNameKind::LookupOrdinaryName); + S.LookupQualifiedName( + NonMembers, + const_cast(EqEqFD->getEnclosingNamespaceContext())); + NonMembers.suppressDiagnostics(); + for (NamedDecl *Op : NonMembers) { + auto *FD = Op->getAsFunction(); + assert(FD->param_size() == 2); + if (S.Context.hasSameUnqualifiedType(EqEqFD->getParamDecl(0)->getType(), + FD->getParamDecl(0)->getType()) && + S.Context.hasSameUnqualifiedType(EqEqFD->getParamDecl(1)->getType(), + FD->getParamDecl(1)->getType())) + return false; + } + return true; +} + +bool OverloadCandidateSet::mayAddReversed(OverloadedOperatorKind Op) { + if (!RewriteInfo.AllowRewrittenCandidates) return false; return Op == OO_EqualEqual || Op == OO_Spaceship; } -bool OverloadCandidateSet::OperatorRewriteInfo::shouldAddReversed( - ASTContext &Ctx, const FunctionDecl *FD) { - if (!shouldAddReversed(FD->getDeclName().getCXXOverloadedOperator())) +bool OverloadCandidateSet::shouldAddReversed(Sema &S, ArrayRef Args, + const FunctionDecl *FD) { + auto Op = FD->getOverloadedOperator(); + if (!mayAddReversed(Op)) + return false; + if (Op == OverloadedOperatorKind::OO_EqualEqual && + !shouldAddReversedEqEq(S, getLocation(), Args, FD)) return false; // Don't bother adding a reversed candidate that can never be a better // match than the non-reversed version. return FD->getNumParams() != 2 || - !Ctx.hasSameUnqualifiedType(FD->getParamDecl(0)->getType(), - FD->getParamDecl(1)->getType()) || + !S.Context.hasSameUnqualifiedType(FD->getParamDecl(0)->getType(), + FD->getParamDecl(1)->getType()) || FD->hasAttr(); } @@ -7720,7 +7782,7 @@ if (FunTmpl) { AddTemplateOverloadCandidate(FunTmpl, F.getPair(), ExplicitTemplateArgs, FunctionArgs, CandidateSet); - if (CandidateSet.getRewriteInfo().shouldAddReversed(Context, FD)) + if (CandidateSet.shouldAddReversed(*this, Args, FD)) AddTemplateOverloadCandidate( FunTmpl, F.getPair(), ExplicitTemplateArgs, {FunctionArgs[1], FunctionArgs[0]}, CandidateSet, false, false, @@ -7729,7 +7791,7 @@ if (ExplicitTemplateArgs) continue; AddOverloadCandidate(FD, F.getPair(), FunctionArgs, CandidateSet); - if (CandidateSet.getRewriteInfo().shouldAddReversed(Context, FD)) + if (CandidateSet.shouldAddReversed(*this, Args, FD)) AddOverloadCandidate(FD, F.getPair(), {FunctionArgs[1], FunctionArgs[0]}, CandidateSet, false, false, true, false, ADLCallKind::NotADL, @@ -7780,12 +7842,17 @@ Operators.suppressDiagnostics(); for (LookupResult::iterator Oper = Operators.begin(), - OperEnd = Operators.end(); - Oper != OperEnd; - ++Oper) + OperEnd = Operators.end(); + Oper != OperEnd; ++Oper) { + if (Oper->getAsFunction() && + PO == OverloadCandidateParamOrder::Reversed && + !CandidateSet.shouldAddReversed(*this, {Args[1], Args[0]}, + Oper->getAsFunction())) + continue; AddMethodCandidate(Oper.getPair(), Args[0]->getType(), Args[0]->Classify(Context), Args.slice(1), CandidateSet, /*SuppressUserConversion=*/false, PO); + } } } @@ -9481,7 +9548,7 @@ FD, FoundDecl, Args, CandidateSet, /*SuppressUserConversions=*/false, PartialOverloading, /*AllowExplicit=*/true, /*AllowExplicitConversion=*/false, ADLCallKind::UsesADL); - if (CandidateSet.getRewriteInfo().shouldAddReversed(Context, FD)) { + if (CandidateSet.shouldAddReversed(*this, Args, FD)) { AddOverloadCandidate( FD, FoundDecl, {Args[1], Args[0]}, CandidateSet, /*SuppressUserConversions=*/false, PartialOverloading, @@ -9494,8 +9561,8 @@ FTD, FoundDecl, ExplicitTemplateArgs, Args, CandidateSet, /*SuppressUserConversions=*/false, PartialOverloading, /*AllowExplicit=*/true, ADLCallKind::UsesADL); - if (CandidateSet.getRewriteInfo().shouldAddReversed( - Context, FTD->getTemplatedDecl())) { + if (CandidateSet.shouldAddReversed(*this, Args, + FTD->getTemplatedDecl())) { AddTemplateOverloadCandidate( FTD, FoundDecl, ExplicitTemplateArgs, {Args[1], Args[0]}, CandidateSet, /*SuppressUserConversions=*/false, PartialOverloading, @@ -13592,14 +13659,14 @@ // Add operator candidates that are member functions. AddMemberOperatorCandidates(Op, OpLoc, Args, CandidateSet); - if (CandidateSet.getRewriteInfo().shouldAddReversed(Op)) + if (CandidateSet.mayAddReversed(Op)) AddMemberOperatorCandidates(Op, OpLoc, {Args[1], Args[0]}, CandidateSet, OverloadCandidateParamOrder::Reversed); // In C++20, also add any rewritten member candidates. if (ExtraOp) { AddMemberOperatorCandidates(ExtraOp, OpLoc, Args, CandidateSet); - if (CandidateSet.getRewriteInfo().shouldAddReversed(ExtraOp)) + if (CandidateSet.mayAddReversed(ExtraOp)) AddMemberOperatorCandidates(ExtraOp, OpLoc, {Args[1], Args[0]}, CandidateSet, OverloadCandidateParamOrder::Reversed); @@ -13775,7 +13842,7 @@ } if (AllowRewrittenCandidates && !IsReversed && - CandidateSet.getRewriteInfo().isReversible()) { + CandidateSet.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 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 --- 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 @@ -135,36 +135,117 @@ }; bool cmp_non_const = A() == A(); // expected-warning {{ambiguous}} - struct B { - virtual bool operator==(const B&) const; - }; - struct D : B { - bool operator==(const B&) const override; // expected-note {{operator}} - }; - bool cmp_base_derived = D() == D(); // expected-warning {{ambiguous}} +} // namespace problem_cases - template struct CRTPBase { - bool operator==(const T&) const; // expected-note {{operator}} expected-note {{reversed}} - bool operator!=(const T&) const; // expected-note {{non-reversed}} - }; - struct CRTP : CRTPBase {}; - 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}} - - // 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}} expected-note {{ambiguity is between}} - }; - bool cmp_icu = ICUDerived() != ICUDerived(); // expected-warning {{ambiguous}} expected-warning {{'bool', not 'UBool'}} +namespace P2468R2 { +//=============Problem cases prior to P2468R2 but now intentionally rejected============= +namespace no_more_problem_cases { +struct B { + virtual bool operator==(const B&) const; +}; +struct D : B { + bool operator==(const B&) const override; // expected-note {{operator}} +}; +bool cmp_base_derived = D() == D(); // expected-warning {{ambiguous}} + +// Reversed "3" not used because we find "2". +// Rewrite != from "3" but warn that "chosen rewritten candidate must return cv-bool". +using UBool = signed char; +struct ICUBase { + virtual UBool operator==(const ICUBase&) const; // 1. + UBool operator!=(const ICUBase &arg) const { return !operator==(arg); } // 2. +}; +struct ICUDerived : ICUBase { + // 3. + UBool operator==(const ICUBase&) const override; // expected-note {{declared here}} +}; +bool cmp_icu = ICUDerived() != ICUDerived(); // expected-warning {{ISO C++20 requires return type of selected 'operator==' function for rewritten '!=' comparison to be 'bool', not 'UBool' (aka 'signed char')}} +} // namespace no_more_problem_cases +//=============Accepted by P2468R2============= +// 1 +struct S { + bool operator==(const S&) { return true; } + bool operator!=(const S&) { return false; } +}; +bool ts = S{} != S{}; +// 2 +template struct CRTPBase { + bool operator==(const T&) const; + bool operator!=(const T&) const; +}; +struct CRTP : CRTPBase {}; +bool cmp_crtp = CRTP() == CRTP(); +bool cmp_crtp2 = CRTP() != CRTP(); +// https://github.com/llvm/llvm-project/issues/57711 +namespace issue_57711 { +template +bool compare(T l, T r) + requires requires { l == r; } { + return l == r; +} + +void test() { + compare(CRTP(), CRTP()); // previously this was a hard error (due to SFINAE failure). +} } +// 3 +template +struct GenericIterator { + using ConstIterator = GenericIterator; + using NonConstIterator = GenericIterator; + GenericIterator() = default; + GenericIterator(const NonConstIterator&); + + bool operator==(ConstIterator) const; + bool operator!=(ConstIterator) const; +}; +using Iterator = GenericIterator; + +bool biter = Iterator{} == Iterator{}; + +//=============Intentionally rejected by P2468R2============= +struct ImplicitInt { + ImplicitInt(); + ImplicitInt(int*); + bool operator==(const ImplicitInt&) const; // expected-note {{reversed}} + operator int*() const; +}; +bool implicit_int = nullptr != ImplicitInt{}; // expected-error {{use of overloaded operator '!=' is ambiguous (with operand types 'std::nullptr_t' and 'ImplicitInt')}} + // expected-note@-1 4 {{built-in candidate operator!=}} + +//============= https://eel.is/c++draft/over.match.oper#example-2 +namespace example { +struct A {}; +template bool operator==(A, T); // 1 +// expected-note@-1 2{{candidate function template not viable: no known conversion from 'int' to 'A' for 1st argument}} +bool a1 = 0 == A(); // OK, calls reversed 1 +template bool operator!=(A, T); +bool a2 = 0 == A(); // expected-error {{invalid operands to binary expression ('int' and 'A')}} + +struct B { + bool operator==(const B&); // 2 + // expected-note@-1 {{ambiguity is between a regular call to this operator and a call with the argument order reversed}} +}; +struct C : B { + C(); + C(B); + bool operator!=(const B&); // 3 +}; +bool c1 = B() == C(); // OK, calls 2; reversed 2 is not a candidate because search for operator!= in C finds #3 +bool c2 = C() == B(); // expected-warning {{ISO C++20 considers use of overloaded operator '==' (with operand types 'C' and 'B') to be ambiguous despite there being a unique best viable function}} + +// FIXME: This should be accepted. != must not be found to allow reversal. +struct D {}; +template bool operator==(D, T); // 4 +// expected-note@-1 {{candidate function template not viable: no known conversion from 'int' to 'D' for 1st argument}} +inline namespace N { + template bool operator!=(D, T); // 5 +} +bool d1 = 0 == D(); // Must be OK, calls reversed 4; 5 does not forbid 4 as a rewrite target +// expected-error@-1 {{invalid operands to binary expression ('int' and 'D')}} +} // namespace example + +} // namespace P2468R2 #else // NO_ERRORS