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

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



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4705
+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<
----------------
NIT: for consistency with other diagnostics.


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


================
Comment at: clang/lib/Sema/SemaOverload.cpp:904
+  for (unsigned I = 0; I < X->getNumParams(); ++I)
+    if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
+                                    Y->getParamDecl(I)->getType()))
----------------
Or, wow, I did not realize that template parameters types for different functions are considered equal. TIL something new, thanks :)


================
Comment at: clang/lib/Sema/SemaOverload.cpp:953
+                          Sema::LookupNameKind::LookupOperatorName);
+  S.LookupName(NonMembers, S.getScopeForContext(const_cast<DeclContext *>(
+                               EqFD->getEnclosingNamespaceContext())));
----------------
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.


================
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();
----------------
NIT: same suggestion as before. Just pass `Expr* FirstOperand` as the parameter instead of an array.


================
Comment at: clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp:129
+namespace P2468R2 {
+//=============Problem cases prior to P2468R2 but now intentionally rejected=============
+struct SymmetricNonConst {
----------------
NIT: same suggestion as before. use plain comments without `====` padding for consistency with the rest of the file.


================
Comment at: clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp:207
+  ImplicitInt(int*);
+  bool operator==(const ImplicitInt&) const; // expected-note {{reversed}}
+  operator int*() const;
----------------
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.


================
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}}
+
----------------
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?


================
Comment at: clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp:238
+}
+bool d1 = 0 == D();  // OK, calls reversed 4; 5 does not forbid 4 as a rewrite target
+} // namespace example
----------------
NIT: could you add a comment mentioning that "search" does not look inside inline namespaces?
This seems non-obvious.


================
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.
+
----------------
Also due to different parameter types (`T` vs `B`)?
So the description is incomplete or am I missing something?


================
Comment at: clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp:275
+bool operator!=(C, int);
+bool c = 0 == C(); // Ok. Use rewritten candidate.
+}
----------------
NIT: add `as the non-template operator != does not correspond to template operator ==`


================
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 {
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529