[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 ↗

(On Diff #462464)

This looks like a leftover from previous version. Remove?

clang/lib/Sema/SemaOverload.cpp

893

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

971

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?

972

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

991

NIT: use early exits:

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

997

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

142

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

893

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

997

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

NIT: for consistency with other diagnostics.

clang/include/clang/Sema/Overload.h

1024

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

904

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

953

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.

976–981

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

129

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

207

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.

231

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?

238

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

269

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

275

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

300

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

1024

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

976–981

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

231

Yes. That is correct.

269

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.

300

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

1024

The new name LG, thanks.

clang/lib/Sema/SemaOverload.cpp

976–981

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

269

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/docs/ReleaseNotes.rst

- Skip rebuilding lambda expressions in arguments of immediate invocations.
   `Issue 51695 <https://github.com/llvm/llvm-project/issues/51695>`_,
   `Issue 50455 <https://github.com/llvm/llvm-project/issues/50455>`_,
   `Issue 54872 <https://github.com/llvm/llvm-project/issues/54872>`_,
   `Issue 54587 <https://github.com/llvm/llvm-project/issues/54587>`_.
 - Clang now correctly delays the instantiation of function constraints until
   the time of checking, which should now allow the libstdc++ ranges implementation
   to work for at least trivial examples.  This fixes
   `Issue 44178 <https://github.com/llvm/llvm-project/issues/44178>`_.
+- Implemented The Equality Operator You Are Looking For (`P2468 <http://wg21.link/p2468r2>`_).
 
 C++2b Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 - Support label at end of compound statement (`P2324 <https://wg21.link/p2324r2>`_).
 
 CUDA/HIP Language Changes in Clang
 ----------------------------------

clang/include/clang/Basic/DiagnosticSemaKinds.td

 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"
   "%select{ with non-reversed arguments|}3">,
   InGroup<DiagGroup<"ambiguous-reversed-operator">>, SFINAEFailure;
 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_eqeq_reversed_self_non_const : Note<
+  "mark operator== as const or add a matching operator!= to resolve the ambiguity">;
 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<
   "overload resolution selected deleted operator '%0'">;

clang/include/clang/Sema/Overload.h

enum CandidateSetKind {
       /// using either a parenthesized or braced list of arguments.
       CSK_InitByConstructor,
     };
 
     /// Information about operator rewrites to consider when adding operator
     /// functions to a candidate set.
     struct OperatorRewriteInfo {
       OperatorRewriteInfo()
-          : OriginalOperator(OO_None), AllowRewrittenCandidates(false) {}
-      OperatorRewriteInfo(OverloadedOperatorKind Op, bool AllowRewritten)
-          : OriginalOperator(Op), AllowRewrittenCandidates(AllowRewritten) {}
+          : OriginalOperator(OO_None), OpLoc(), AllowRewrittenCandidates(false) {}
+      OperatorRewriteInfo(OverloadedOperatorKind Op, SourceLocation OpLoc,
+                          bool AllowRewritten)
+          : OriginalOperator(Op), OpLoc(OpLoc),
+            AllowRewrittenCandidates(AllowRewritten) {}
 
       /// The original operator as written in the source.
       OverloadedOperatorKind OriginalOperator;
+      /// The source location of the operator.
+      SourceLocation OpLoc;
       /// Whether we should include rewritten candidates in the overload set.
       bool AllowRewrittenCandidates;
 
       /// Would use of this function result in a rewrite using a different
       /// operator?
       bool isRewrittenOperator(const FunctionDecl *FD) {
         return OriginalOperator &&
                FD->getDeclName().getCXXOverloadedOperator() != OriginalOperator;
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));
+                mayAddReversed(OriginalOperator));
       }
 
-      /// Determine whether we should consider looking for and adding reversed
+      /// Determine whether we may consider looking for and adding reversed
       /// candidates for operator Op.
-      bool shouldAddReversed(OverloadedOperatorKind 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);
+      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.

clang/lib/Sema/SemaDeclCXX.cpp

visitBinaryOperator(OverloadedOperatorKind OO, ArrayRef<Expr *> Args,
                       Subobject Subobj,
                       OverloadCandidateSet *SpaceshipCandidates = nullptr) {
     // Note that there is no need to consider rewritten candidates here if
     // we've already found there is no viable 'operator<=>' candidate (and are
     // considering synthesizing a '<=>' from '==' and '<').
     OverloadCandidateSet CandidateSet(
         FD->getLocation(), OverloadCandidateSet::CSK_Operator,
         OverloadCandidateSet::OperatorRewriteInfo(
-            OO, /*AllowRewrittenCandidates=*/!SpaceshipCandidates));
+            OO, FD->getLocation(),
+            /*AllowRewrittenCandidates=*/!SpaceshipCandidates));
 
     /// C++2a [class.compare.default]p1 [P2002R0]:
     ///   [...] the defaulted function itself is never a candidate for overload
     ///   resolution [...]
     CandidateSet.exclude(FD);
 
     if (Args[0]->getType()->isOverloadableType())
       S.LookupOverloadedBinOp(CandidateSet, OO, Fns, Args);

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(
+static bool FunctionsCorrespond(ASTContext &Ctx, const FunctionDecl *X,
+                                const FunctionDecl *Y) {
+  if (!X || !Y)
+    return false;
+  if (X->getNumParams() != Y->getNumParams())
+    return false;
+  for (unsigned I = 0; I < X->getNumParams(); ++I)
+    if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
+                                    Y->getParamDecl(I)->getType()))
+      return false;
+  if (auto *FTX = X->getDescribedFunctionTemplate()) {
+    auto *FTY = Y->getDescribedFunctionTemplate();
+    if (!FTY)
+      return false;
+    if (!Ctx.isSameTemplateParameterList(FTX->getTemplateParameters(),
+                                         FTY->getTemplateParameters()))
+      return false;
+  }
+  return true;
+}
+
+static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc,
+                                  Expr *FirstOperand,
+                                  const FunctionDecl *EqFD) {
+  assert(EqFD->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>(EqFD)) {
+    // If F is a class member, search scope is class type of first operand.
+    QualType RHS = FirstOperand->getType();
+    auto *RHSRec = RHS->getAs<RecordType>();
+    if (!RHSRec)
+      return true;
+    LookupResult Members(S, NotEqOp, OpLoc,
+                         Sema::LookupNameKind::LookupMemberName);
+    S.LookupQualifiedName(Members, RHSRec->getDecl());
+    Members.suppressDiagnostics();
+    for (NamedDecl *Op : Members)
+      if (FunctionsCorrespond(S.Context, EqFD, Op->getAsFunction()))
+        return false;
+    return true;
+  }
+  // Otherwise the search scope is the namespace scope of which F is a member.
+  LookupResult NonMembers(S, NotEqOp, OpLoc,
+                          Sema::LookupNameKind::LookupOperatorName);
+  S.LookupName(NonMembers, S.getScopeForContext(const_cast<DeclContext *>(
+                               EqFD->getEnclosingNamespaceContext())));
+  NonMembers.suppressDiagnostics();
+  for (NamedDecl *Op : NonMembers) {
+    auto *FD = Op->getAsFunction();
+    if(auto* UD = dyn_cast<UsingShadowDecl>(Op))
+      FD = UD->getUnderlyingDecl()->getAsFunction();
+    if (FunctionsCorrespond(S.Context, EqFD, FD) &&
+        declaresSameEntity(cast<Decl>(EqFD->getDeclContext()),
+                           cast<Decl>(Op->getDeclContext())))
+      return false;
+  }
+  return true;
+}
+
+bool OverloadCandidateSet::OperatorRewriteInfo::mayAddReversed(
     OverloadedOperatorKind Op) {
   if (!AllowRewrittenCandidates)
     return false;
   return Op == OO_EqualEqual || Op == OO_Spaceship;
 }
 
 bool OverloadCandidateSet::OperatorRewriteInfo::shouldAddReversed(
-    ASTContext &Ctx, const FunctionDecl *FD) {
-  if (!shouldAddReversed(FD->getDeclName().getCXXOverloadedOperator()))
+    Sema &S, ArrayRef<Expr *> Args, const FunctionDecl *FD) {
+  auto Op = FD->getOverloadedOperator();
+  if (!mayAddReversed(Op))
+    return false;
+  if (Op == OverloadedOperatorKind::OO_EqualEqual &&
+      !shouldAddReversedEqEq(S, OpLoc, Args[1], 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)
       i->DeductionFailure.Destroy();
   }
 }
 
 void OverloadCandidateSet::clear(CandidateSetKind CSK) {
   destroyCandidates();
   SlabAllocator.Reset();
   NumInlineBytesUsed = 0;
   Candidates.clear();
   Functions.clear();
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.getRewriteInfo().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.getRewriteInfo().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.getRewriteInfo().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.getRewriteInfo().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())) {
+              *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.getRewriteInfo().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.getRewriteInfo().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).
if (Opc == BO_Assign && !Args[0]->getType()->isOverloadableType())
     return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
 
   // If this is the .* operator, which is not overloadable, just
   // create a built-in binary operator.
   if (Opc == BO_PtrMemD)
     return CreateBuiltinBinOp(OpLoc, Opc, Args[0], Args[1]);
 
   // Build the overload set.
-  OverloadCandidateSet CandidateSet(
-      OpLoc, OverloadCandidateSet::CSK_Operator,
-      OverloadCandidateSet::OperatorRewriteInfo(Op, AllowRewrittenCandidates));
+  OverloadCandidateSet CandidateSet(OpLoc, OverloadCandidateSet::CSK_Operator,
+                                    OverloadCandidateSet::OperatorRewriteInfo(
+                                        Op, OpLoc, AllowRewrittenCandidates));
   if (DefaultedFn)
     CandidateSet.exclude(DefaultedFn);
   LookupOverloadedBinOp(CandidateSet, Op, Fns, Args, PerformADL);
 
   bool HadMultipleCandidates = (CandidateSet.size() > 1);
 
   // Perform overload resolution.
   OverloadCandidateSet::iterator Best;
case OR_Success: {
                 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);
+              // Mark member== const or provide matching != to disallow reversed
+              // args. Eg. 
+              // struct S { bool operator==(const S&); }; 
+              // S()==S();
+              if (auto *MD = dyn_cast<CXXMethodDecl>(FnDecl))
+                if (Op == OverloadedOperatorKind::OO_EqualEqual &&
+                    !MD->isConst() &&
+                    Context.hasSameUnqualifiedType(
+                        MD->getThisObjectType(),
+                        MD->getParamDecl(0)->getType().getNonReferenceType()) &&
+                    Context.hasSameUnqualifiedType(MD->getThisObjectType(),
+                                                   Args[0]->getType()) &&
+                    Context.hasSameUnqualifiedType(MD->getThisObjectType(),
+                                                   Args[1]->getType()))
+                  Diag(FnDecl->getLocation(),
+                       diag::note_ovl_ambiguous_eqeq_reversed_self_non_const);
             } 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);
             }
           }

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

bool operator==(type lhs, int rhs) {
       return true;
     }
   }
 
   bool b1 = ADL::type() == 0;
   bool b2 = 0 == ADL::type();
 }
 
-// Various C++17 cases that are known to be broken by the C++20 rules.
-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}}
+namespace P2468R2 {
+//=============Problem cases prior to P2468R2 but now intentionally rejected=============
+struct SymmetricNonConst {
+  bool operator==(const SymmetricNonConst&); // expected-note {{ambiguity is between a regular call to this operator and a call with the argument order reversed}}
+  // expected-note@-1 {{mark operator== as const or add a matching operator!= to resolve the ambiguity}}
+bool cmp_non_const = SymmetricNonConst() == SymmetricNonConst(); // expected-warning {{ambiguous}}
+
+struct SymmetricConst {
+  bool operator==(const SymmetricConst&) const;
+};
+bool cmp_const = SymmetricConst() == SymmetricConst();
 
-  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}}
 
-  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')}}
+// 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 {{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}}
+
+struct D {};
+template<typename T> bool operator==(D, T);     // 4
+inline namespace N {
+  template<typename T> bool operator!=(D, T);   // 5
+}
+bool d1 = 0 == D();  // OK, calls reversed 4; 5 does not forbid 4 as a rewrite target
+} // namespace example
+
+namespace template_tests {
+namespace template_head_does_not_match {
+struct A {};
+template<typename T, class U = int> bool operator==(A, T);
+template <class T> bool operator!=(A, T);
+bool x = 0 == A(); // Ok. Use rewritten candidate.
+}
+
+namespace template_with_different_param_name_are_equivalent {
+struct A {};
+template<typename T> bool operator==(A, T); // expected-note {{candidate function template not viable: no known conversion from 'int' to 'A' for 1st argument}}
+template <typename U> bool operator!=(A, U);
+bool x = 0 == A(); // expected-error {{invalid operands to binary expression ('int' and 'A')}}
+}
+
+namespace template_and_non_template {
+struct A {
+template<typename T> bool operator==(const T&);
+// expected-note@-1{{mark operator== as const or add a matching operator!= to resolve the ambiguity}}
+// expected-note@-2{{ambiguity is between a regular call to this operator and a call with the argument order reversed}}
+bool operator!=(const A&); // does not forbid rewrite.
+};
+bool a = A() == A(); // expected-warning {{ ambiguous despite there being a unique best viable function}}
+
+struct B {
+template<typename T> bool operator==(const T&) const;
+bool operator!=(const B&);
+};
+bool b = B() == B(); // ok. No rewrite due to const.
+
+struct C {};
+template <class T=int>
+bool operator==(C, int);
+bool operator!=(C, int);
+bool c = 0 == C(); // Ok. Use rewritten candidate.
+}
+} // template_tests
+
+namespace using_decls {
+namespace simple {
+struct C {};
+bool operator==(C, int); // expected-note {{candidate function not viable: no known conversion from 'int' to 'C' for 1st argument}}
+bool a = 0 == C(); // Ok. Use rewritten candidate.
+namespace other_ns { bool operator!=(C, int); }
+bool b = 0 == C(); // Ok. Use rewritten candidate.
+using other_ns::operator!=;
+bool c = 0 == C(); // Rewrite not possible. expected-error {{invalid operands to binary expression ('int' and 'C')}}
 }
+namespace templated {
+struct C {};
+template<typename T>
+bool operator==(C, T); // expected-note {{candidate function template not viable: no known conversion from 'int' to 'C' for 1st argument}}
+bool a = 0 == C(); // Ok. Use rewritten candidate.
+namespace other_ns { template<typename T> bool operator!=(C, T); }
+bool b = 0 == C(); // Ok. Use rewritten candidate.
+using other_ns::operator!=;
+bool c = 0 == C(); // Rewrite not possible. expected-error {{invalid operands to binary expression ('int' and 'C')}}
+} // templated
+} // using_decls
+// FIXME: Match requires clause.
+// namespace match_requires_clause {
+// template<int x>
+// struct A {
+// bool operator==(const A&) requires (x==1);
+// bool operator!=(const A&) requires (x==1);
+// };
+// int a1 = A<1>() == A<1>();
+// }
+} // 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;