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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM with a few minor NITs



================
Comment at: clang/include/clang/Sema/Overload.h:1024
       /// candidates for operator Op.
-      bool shouldAddReversed(OverloadedOperatorKind Op);
+      bool mayAddReversed(OverloadedOperatorKind Op);
 
----------------
usaxena95 wrote:
> 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.
The new name LG, thanks.


================
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();
----------------
usaxena95 wrote:
> 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`.
> 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)`.



================
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.
+
----------------
usaxena95 wrote:
> 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.
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](https://eel.is/c++draft/over.match#best.general-2.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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529