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