[C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[usaxena95]

Summary

Implement
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2468r2.html.

Primarily we now accept

template<typename T> struct CRTPBase {
  bool operator==(const T&) const;
  bool operator!=(const T&) const;
};
struct CRTP : CRTPBase<CRTP> {};
bool cmp_crtp = CRTP() == CRTP();
bool cmp_crtp2 = CRTP() != CRTP();

Fixes: https://github.com/llvm/llvm-project/issues/57711

[usaxena95]

Added better diagnostic for non-const symmetric operator==.
Added release notes.
Updated tests.

[ilya-biryukov]

A few ideas for tests:

• Static operators. Technically disallowed by the standard, but Clang seems to recover without dropping the member, so it seems we can actually look it up.

struct X { 
  bool operator ==(X const&) const;
  static bool operator !=(X const&, X const&);
};

this could potentially lead to a crash if the parameter list for the operator is empty:

struct X { 
  bool operator ==(X const&) const;
  static bool operator !=();
};

• template functions with non-matching template heads (so they don't "correspond")

template <class T, class U = int>
bool operator ==(T, T);
template <class T>
bool operator !=(T, T)

• template function and non-template function:

template <class T= int>
bool operator ==(X, X);
bool operator !=(X, X);

clang/include/clang/Sema/Sema.h

3768

This looks like a leftover from previous version. Remove?

clang/lib/Sema/SemaOverload.cpp

887

Why do we need to move this from OperatorRewriteInfo to OverloadCandidateSet?

892

We only use Args[1], let's pass Expr* instead of an array.
The standard calls this expression "first operand", so let's sync the naming and accept the Expr *FirstOperand?

893

NIT: maybe rename to EqFD?
EqEqFD is somewhat hard to grasp and I don't think there any possibility for confusion with assignment here.

912

NIT: use early exits:

auto *RHSRec = RHS->getAs<RecordType>();
if (!RHSRec)
  return true;
// <else branch with reduced nesting>

918

Could we implement the "corresponds" check from (basic.scope.scope)p4 directly?

This should address the existing FIXMEs about const members and template functions.

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp

141

NIT: the rest of the file uses plain comments. Maybe stick to existing style and avoid heading padded with =====

[ilya-biryukov]

Overall LG, thanks!
The only major comment from me is that we probably want to implement the full "corresponds" check so we handle various cases mentioned in the FIXMEs.

Also adding the Language WG as reviewers in case someone else wants to take a look.

I don't follow why this disallows the reverse #4.

It seems the trick is to pick the right type of the lookup.
The particular wording says to search for a name in scope rather than do a qualified lookup.
Only the latter should look inside the qualified namespaces.

Another idea for a test: try replacing declarations of operator!= with using other_ns::operator!= in various examples and make sure that it keeps working.

[usaxena95]

Addressed comments.

[usaxena95]

Added test for static operator.

[usaxena95]

clang/lib/Sema/SemaOverload.cpp

887

We are using operator location for search which is part of OverloadCandidateSet.
I propagated it to the RewriteInfo without shuffling these declarations.

918

Implemented the corresponds and search through LookupName.
Needs additional filtering based on matching DeclContext to verify they are from the same namespace.

I was not able to find any utility which facilitates "search" out of the box. Does this look good ?

[ilya-biryukov]

Thanks! I have only various code style comments here, ptal. Overall LG.

clang/include/clang/Basic/DiagnosticSemaKinds.td

4705 ↗

(On Diff #464012)

NIT: for consistency with other diagnostics.

clang/include/clang/Sema/Overload.h

1020–1022

I am not sure the new name adds clarity.
It's unclear what the true return value means here. should clearly indicated returning true means the code has to proceed with adding the reversed operator. may means the code can choose to do so or not, I don't think that's quite right. should was really a better choice here.

That said, I don't think the rename itself is a bad idea, maybe there is a better name, but I couldn't come up with one.

clang/lib/Sema/SemaOverload.cpp

897

Or, wow, I did not realize that template parameters types for different functions are considered equal. TIL something new, thanks :)

946

NIT: remove const_cast, the non-const FunctionDecl* instead.
You'd need to do this for the corresponding parameter in shouldAddReversed too, but that seems very easy.

952–958

NIT: same suggestion as before. Just pass Expr* FirstOperand as the parameter instead of an array.

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp

137

NIT: same suggestion as before. use plain comments without ==== padding for consistency with the rest of the file.

206

NIT: could we match against a larger piece of the message?
it's hard to read what the test is trying to check here.
In particular, I'm not sure if this intends to show the newly added note or simply point to operator that had its arguments reversed.

230

NIT: could you add a comment explaining why this is ambiguous? This seems non-obvious.
It's because the search for operator!= happens inside B and never finds 3, right?

237

NIT: could you add a comment mentioning that "search" does not look inside inline namespaces?
This seems non-obvious.

268

Also due to different parameter types (T vs B)?
So the description is incomplete or am I missing something?

274

NIT: add as the non-template operator != does not correspond to template operator ==

299

NIT: maybe file a bug for this and mention the GH issue number?
(could be done in the last iteration right before landing the change)

[usaxena95]

Addressed comments.

[usaxena95]

clang/include/clang/Sema/Overload.h

1020–1022

Just to be clear, it is possible to not reverse the args even if this returns true now since we do not do full check for == here.

I renamed it allowsReversed on the same line of AllowRewrittenCandidates.

Intention is to convey that if this is true then reversing is allowed and you can choose not to do so as well.

clang/lib/Sema/SemaOverload.cpp

952–958

I would prefer not to use a FirstOperand for shouldAddReversed as it serves more than just operator==.

It might be confusing/error-prone for the users as to what is the "first operand" here (Args[0] or Args[1] ? In reversed args or original args ?). I think it would be a better API to just have original args as the parameter let this function decide which is the FirstOperand.

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp

230

Yes. That is correct.

268

This verifies that adding const would solve the ambiguity without adding a matching !=. The != here does not match because it is non-template and, yes, because of different param types.

299

Sure. I can do this in a followup patch once this issue lands.

[ilya-biryukov]

LGTM with a few minor NITs

clang/include/clang/Sema/Overload.h

1020–1022

The new name LG, thanks.

clang/lib/Sema/SemaOverload.cpp

952–958

It might be confusing/error-prone for the users as to what is the "first operand" here (Args[0] or Args[1] ?

Yeah, good point. It's much better to have this choice in this function to avoid the complicated contract on each call site.
That being said...

let this function decide which is the FirstOperand.

We should have a comment explaining why *first* operand is Args[1]. It's a bit non-obvious, but followed from the specification.

I also suggest to add an extra sanity check: assert(OriginalArgs.size() == 2).

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp

268

However, the rewritten operator does get added in that case, right?
So my reading is that the 'rewrite' does happen, but since conversions are the same, the
(over.match.best.genera)p2.8 kick in and there is no ambiguity:

Given these definitions, a viable function F1 is defined to be a better function than another viable function F2 ...

• F2 is a rewritten candidate ([over.match.oper]) and F1 is not

I suggest to:

• remove the operator != completely, we check having it does not block adding rewritten operators in other tests,
• change the comment to say No ambiguity due to const as the rewrite actually does happen.

[usaxena95]

Addressed comments.

[royjacobson]

Should cxx_status.html be updated as well? It's listed there as a C++2b paper.

[sberg]

I just ran into newly-failing

$ cat test.cc
struct S1 {
    bool operator==(int);
    bool operator!=(int);
};
struct S2;
bool operator ==(S2, S2);
bool f(S1 s) { return 0 == s; }

$ clang++ -std=c++20 -fsyntax-only test.cc
test.cc:7:25: error: invalid operands to binary expression ('int' and 'S1')
bool f(S1 s) { return 0 == s; }
                      ~ ^  ~
test.cc:6:6: note: candidate function not viable: no known conversion from 'int' to 'S2' for 1st argument
bool operator ==(S2, S2);
     ^
1 error generated.

It looks to me like this is correctly rejected now per P2468R2. But it is rather confusing that a note mentions the S2 candidate, while the relevant S1 operators == and != are not mentioned at all.

[ilya-biryukov]

In D134529#3845617, @sberg wrote:

I just ran into newly-failing
It looks to me like this is correctly rejected now per P2468R2.

Yes, this is intentional, the behavior of the code is the same as in C++17.

But it is rather confusing that a note mentions the S2 candidate, while the relevant S1 operators == and != are not mentioned at all.

The diagnostic is indeed not very helpful and it would be nice to improve it.
However, this is unrelated to this change, e.g. it reproduces in C++17 mode with Clang 10.

In D134529#3845050, @royjacobson wrote:

Should cxx_status.html be updated as well? It's listed there as a C++2b paper.

@usaxena95 we have forgotten to do this indeed. Could you please update the relevant html file?

[usaxena95]

Sent out https://reviews.llvm.org/D135608 to update implementation status.

[erichkeane]

Note that @BertalanD noticed an issue with what I expect to be this patch: https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an intended change? It seems to me that the reversing behavior of the equality operators is a little awkward here (note that commenting out != makes that 'work' again).

[ilya-biryukov]

In D134529#3852990, @erichkeane wrote:

Note that @BertalanD noticed an issue with what I expect to be this patch: https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an intended change? It seems to me that the reversing behavior of the equality operators is a little awkward here (note that commenting out != makes that 'work' again).

This is the intended behaviour, yes. The paper aims to revert back to C++17 behavior when we see a matching operator != and this was an error in C++17: https://godbolt.org/z/K3cr9MMh5.
As mentioned above, the actual compiler error message is not very helpful and is something that could be improved, but this was not introduced by this patch.

[erichkeane]

In D134529#3853036, @ilya-biryukov wrote:

In D134529#3852990, @erichkeane wrote:

Note that @BertalanD noticed an issue with what I expect to be this patch: https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an intended change? It seems to me that the reversing behavior of the equality operators is a little awkward here (note that commenting out != makes that 'work' again).

This is the intended behaviour, yes. The paper aims to revert back to C++17 behavior when we see a matching operator != and this was an error in C++17: https://godbolt.org/z/K3cr9MMh5.
As mentioned above, the actual compiler error message is not very helpful and is something that could be improved, but this was not introduced by this patch.

Thank you very much for clarifying! I wasn't sure, and didn't want to mislead @BertalanD.

[usaxena95]

And Ilya beat me to it.

In D134529#3852990, @erichkeane wrote:

Note that @BertalanD noticed an issue with what I expect to be this patch: https://godbolt.org/z/Wjb9rsEYG

Can someone more knowledgable about this paper please make sure it is an intended change? It seems to me that the reversing behavior of the equality operators is a little awkward here (note that commenting out != makes that 'work' again).

This is intentional. Example from https://eel.is/c++draft/over.match.oper#4

struct A {};
template<typename T> bool operator==(A, T);     // #1
bool a1 = 0 == A();                             // OK, calls reversed #1
template<typename T> bool operator!=(A, T);
bool a2 = 0 == A();                             // error, #1 is not a rewrite target

Adding a matching operator!= disables reversing arguments (C++17 mode). The LHS in this case should provide a operator==.

The diagnostic might not be the most helpful at this point is. You can choose to

• Remove operator!=. Allows assuming x==y is equivalent to y==x and therefore uses the same function with reversed arguments.
• Write s == c instead of c == s.

[BertalanD]

Thank you all for the help and clarification! I'll go fix our code then.

As an aside, I understand that this paper now retroactively applies to the C++ standard, but code has already been written that assumes that what my example is doing is valid. Does it deserve a mention in the Potentially Breaking Changes section of the release notes?

[usaxena95]

In D134529#3853086, @BertalanD wrote:

Does it deserve a mention in the Potentially Breaking Changes section of the release notes?

You are right. I was thinking the same after this discussion. I will add this to the section.

clang/include/clang/Sema/Overload.h

struct OperatorRewriteInfo {
       getRewriteKind(const FunctionDecl *FD, OverloadCandidateParamOrder PO) {
         OverloadCandidateRewriteKind CRK = CRK_None;
         if (isRewrittenOperator(FD))
           CRK = OverloadCandidateRewriteKind(CRK | CRK_DifferentOperator);
         if (PO == OverloadCandidateParamOrder::Reversed)
           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));
-      }
+      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
+    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);
-    };
+    bool shouldAddReversed(Sema &S, ArrayRef<Expr *> Args, const FunctionDecl *FD);
 
   private:
     SmallVector<OverloadCandidate, 16> Candidates;
     llvm::SmallPtrSet<uintptr_t, 16> Functions;
 
     // Allocator for ConversionSequenceLists. We store the first few of these
     // inline to avoid allocation for small sets.
     llvm::BumpPtrAllocator SlabAllocator;

clang/include/clang/Sema/Sema.h

enum CCEKind {
     CCEK_ExplicitBool, ///< Condition in an explicit(bool) specifier.
     CCEK_Noexcept      ///< Condition in a noexcept(bool) specifier.
   };
   ExprResult CheckConvertedConstantExpression(Expr *From, QualType T,
                                               llvm::APSInt &Value, CCEKind CCE);
   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 {
   public:
     bool Suppress;
     bool SuppressConversion;
 
     ContextualImplicitConverter(bool Suppress = false,

clang/lib/Sema/SemaOverload.cpp

 //===--- SemaOverload.cpp - C++ Overloading -------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides Sema routines for C++ overloading.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Casting.h"
 #include <algorithm>
 #include <cstdlib>
 
 using namespace clang;
 using namespace sema;
 
 using AllowedExplicit = Sema::AllowedExplicit;
 
llvm::Optional<unsigned> DeductionFailureInfo::getCallArgIndex() {
   case Sema::TDK_DeducedMismatchNested:
     return static_cast<DFIDeducedMismatchArgs*>(Data)->CallArgIndex;
 
   default:
     return llvm::None;
   }
 }
 
-bool OverloadCandidateSet::OperatorRewriteInfo::shouldAddReversed(
-    OverloadedOperatorKind Op) {
-  if (!AllowRewrittenCandidates)
+static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc,
+                                  ArrayRef<Expr *> 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<CXXMethodDecl>(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<RecordType>()) {
+      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<DeclContext *>(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<Expr *> 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(),
+         !S.Context.hasSameUnqualifiedType(FD->getParamDecl(0)->getType(),
                                            FD->getParamDecl(1)->getType()) ||
          FD->hasAttr<EnableIfAttr>();
 }
 
 void OverloadCandidateSet::destroyCandidates() {
   for (iterator i = begin(), e = end(); i != e; ++i) {
     for (auto &C : i->Conversions)
       C.~ImplicitConversionSequence();
     if (!i->Viable && i->FailureKind == ovl_fail_bad_deduction)
if (!CandidateSet.getRewriteInfo().isAcceptableCandidate(FD))
       continue;
 
     assert(!isa<CXXMethodDecl>(FD) &&
            "unqualified operator lookup found a member function");
 
     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,
             true, ADLCallKind::NotADL, OverloadCandidateParamOrder::Reversed);
     } else {
       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,
                              None, OverloadCandidateParamOrder::Reversed);
     }
   }
 }
 
if (const RecordType *T1Rec = T1->getAs<RecordType>()) {
     if (!T1Rec->getDecl()->getDefinition())
       return;
 
     LookupResult Operators(*this, OpName, OpLoc, LookupOrdinaryName);
     LookupQualifiedName(Operators, T1Rec->getDecl());
     Operators.suppressDiagnostics();
 
     for (LookupResult::iterator Oper = Operators.begin(),
                                 OperEnd = Operators.end();
-         Oper != OperEnd;
-         ++Oper)
+         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);
     }
   }
+}
 
 /// AddBuiltinCandidate - Add a candidate for a built-in
 /// operator. ResultTy and ParamTys are the result and parameter types
 /// of the built-in candidate, respectively. Args and NumArgs are the
 /// arguments being passed to the candidate. IsAssignmentOperator
 /// should be true when this built-in candidate is an assignment
 /// operator. NumContextualBoolArguments is the number of arguments
 /// (at the beginning of the argument list) that will be contextually
for (ADLResult::iterator I = Fns.begin(), E = Fns.end(); I != E; ++I) {
     if (FunctionDecl *FD = dyn_cast<FunctionDecl>(*I)) {
       if (ExplicitTemplateArgs)
         continue;
 
       AddOverloadCandidate(
           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,
             /*AllowExplicit=*/true, /*AllowExplicitConversion=*/false,
             ADLCallKind::UsesADL, None, OverloadCandidateParamOrder::Reversed);
       }
     } else {
       auto *FTD = cast<FunctionTemplateDecl>(*I);
       AddTemplateOverloadCandidate(
           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,
             /*AllowExplicit=*/true, ADLCallKind::UsesADL,
             OverloadCandidateParamOrder::Reversed);
       }
     }
   }
OverloadedOperatorKind ExtraOp =
           : OO_None;
 
   // Add the candidates from the given function set. This also adds the
   // rewritten candidates using these functions if necessary.
   AddNonMemberOperatorCandidates(Fns, Args, CandidateSet);
 
   // 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);
   }
 
   // Add candidates from ADL. Per [over.match.oper]p2, this lookup is not
   // performed for an assignment operator (nor for operator[] nor operator->,
   // which don't get here).
case OR_Success: {
               << FnDecl->getReturnType() << BinaryOperator::getOpcodeStr(Opc)
               << Args[0]->getSourceRange() << Args[1]->getSourceRange();
           Diag(FnDecl->getLocation(), diag::note_declared_at);
           if (!IsExtension)
             return ExprError();
         }
 
         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
           // formally ambiguous, and allowing it is an extension.
           llvm::SmallVector<FunctionDecl*, 4> AmbiguousWith;
           for (OverloadCandidate &Cand : CandidateSet) {
             if (Cand.Viable && Cand.Function && Cand.isReversed() &&
                 haveSameParameterTypes(Context, Cand.Function, FnDecl, 2)) {

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp

 namespace problem_cases {
   // We can have an ambiguity between an operator and its reversed form. This
   // wasn't intended by the original "consistent comparison" proposal, and we
   // allow it as extension, picking the non-reversed form.
   struct A {
     bool operator==(const A&); // expected-note {{ambiguity is between a regular call to this operator and a call with the argument order reversed}}
   };
   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
+
+namespace P2468R2 {
+//=============Problem cases prior to P2468R2 but now intentionally rejected=============
+namespace no_more_problem_cases {
 
-  template<typename T> 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<CRTP> {};
-  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}}
+// 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
+  bool operator==(const T&) const;
+  bool operator!=(const T&) const;
+bool cmp_crtp = CRTP() == CRTP();
+bool cmp_crtp2 = CRTP() != CRTP();
+// https://github.com/llvm/llvm-project/issues/57711
+namespace issue_57711 {
+template <class T>
+bool compare(T l, T r)
+    requires requires { l == r; } {
+  return l == r;
+}
 
-  // 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'}}
+void test() {
+  compare(CRTP(), CRTP()); // previously this was a hard error (due to SFINAE failure).
+}
+}
+// 3
+template <bool>
+struct GenericIterator {
+  using ConstIterator = GenericIterator<true>;
+  using NonConstIterator = GenericIterator<false>;
+  GenericIterator() = default;
+  GenericIterator(const NonConstIterator&);
+
+  bool operator==(ConstIterator) const;
+  bool operator!=(ConstIterator) const;
+using Iterator = GenericIterator<false>;
+
+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<typename T> 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<typename T> 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<typename T> 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<typename T> 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
 
 namespace problem_cases {
   // We can select a reversed candidate where we used to select a non-reversed
   // one, and in the worst case this can dramatically change the meaning of the
   // program. Make sure we at least warn on the worst cases under -Wall.
   struct iterator;
   struct const_iterator {
     const_iterator(iterator);
     bool operator==(const const_iterator&) const;
   };
   struct iterator {
     bool operator==(const const_iterator &o) const { // expected-warning {{all paths through this function will call itself}}
       return o == *this;
     }
   };
 }
 #endif // NO_ERRORS