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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
