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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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")

  bool operator ==(X, X);



================
Comment at: clang/include/clang/Sema/Sema.h:3768
                                               NamedDecl *Dest = nullptr);
-
+  // bool HasSameExclaimEqual(const FunctionDecl *FD, SourceLocation OpLoc,
+  //                          Expr *RHSArg);
----------------
This looks like a leftover from previous version. Remove?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:887
 
-bool OverloadCandidateSet::OperatorRewriteInfo::shouldAddReversed(
-    OverloadedOperatorKind Op) {
----------------
Why do we need to move this from `OperatorRewriteInfo` to `OverloadCandidateSet`?



================
Comment at: clang/lib/Sema/SemaOverload.cpp:892
+static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc,
+                                  ArrayRef<Expr *> Args,
+                                  const FunctionDecl *EqEqFD) {
----------------
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`?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:893
+                                  ArrayRef<Expr *> Args,
+                                  const FunctionDecl *EqEqFD) {
+  assert(EqEqFD->getOverloadedOperator() ==
----------------
NIT: maybe rename to `EqFD`?
`EqEqFD` is somewhat hard to grasp and I don't think there any possibility for confusion with assignment here.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:912
+    QualType RHS = Args[1]->getType();
+    if (auto *RHSRec = RHS->getAs<RecordType>()) {
+      LookupResult Members(S, NotEqOp, OpLoc,
----------------
NIT:  use [early exits](https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code):
```
auto *RHSRec = RHS->getAs<RecordType>();
if (!RHSRec)
  return true;
// <else branch with reduced nesting>
```


================
Comment at: clang/lib/Sema/SemaOverload.cpp:918
+      for (NamedDecl *Op : Members)
+        if (S.Context.hasSameUnqualifiedType(
+                MD->getParamDecl(0)->getType(),
----------------
Could we implement the "corresponds" check from [(basic.scope.scope)p4](https://eel.is/c++draft/basic.scope.scope) directly?

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


================
Comment at: clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp:141
+namespace P2468R2 {
+//=============Problem cases prior to P2468R2 but now intentionally rejected=============
+namespace no_more_problem_cases {
----------------
NIT: the rest of the file uses plain comments. Maybe stick to existing style and avoid heading padded with `=====`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134529/new/

https://reviews.llvm.org/D134529