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

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added inline comments.


================
Comment at: clang/include/clang/Sema/Overload.h:1024
       /// candidates for operator Op.
-      bool shouldAddReversed(OverloadedOperatorKind Op);
+      bool mayAddReversed(OverloadedOperatorKind Op);
 
----------------
ilya-biryukov wrote:
> 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.
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.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:976
 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();
----------------
ilya-biryukov wrote:
> NIT: same suggestion as before. Just pass `Expr* FirstOperand` as the parameter instead of an array.
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`.


================
Comment at: clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp:231
+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}}
+
----------------
ilya-biryukov wrote:
> 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?
Yes. That is correct.


================
Comment at: clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp:269
+};
+bool b = B() == B(); // ok. No rewrite due to const.
+
----------------
ilya-biryukov wrote:
> Also due to different parameter types (`T` vs `B`)?
> So the description is incomplete or am I missing something?
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.


================
Comment at: clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp:300
+} // using_decls
+// FIXME: Match requires clause.
+// namespace match_requires_clause {
----------------
ilya-biryukov wrote:
> 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)
Sure. I can do this in a followup patch once this issue lands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529