rsmith added inline comments.
================
Comment at: lib/Sema/SemaOverload.cpp:839
+
+unsigned OverloadCandidate::getTrueArgIndex(unsigned Idx) const {
+ if (getRewrittenKind() != ROC_Synthesized)
----------------
I think this might be clearer if named `getParamIndexForArgIndex` or similar.
================
Comment at: lib/Sema/SemaOverload.cpp:8880-8882
+/// \brief Add the rewritten and synthesized candidates for binary comparison
+// operators. No additional semantic checking is done to see if the
candidate
+// is well formed.
----------------
No `\brief` here (we use autobrief), and please use `///` for continuation
lines.
================
Comment at: lib/Sema/SemaOverload.cpp:8888
+ bool PerformADL) {
+ assert(getLangOpts().CPlusPlus2a);
+ auto Opc = BinaryOperator::getOverloadedOpcode(Op);
----------------
Does the implementation below actually rely on this?
================
Comment at: lib/Sema/SemaOverload.cpp:8901
+ // Lookup possible candidates for the rewritten operator.
+ // FIXME: should this really be done in the current scope?
+ LookupResult Operators(*this, OpName, SourceLocation(),
----------------
No, you shouldn't be doing any lookups here. Instead, what you should do is to
change the `LookupOverloadedOperatorName` call in `BuildOverloadedBinOp` to
look up both `operator@` and `operator<=>` in the case where `@` is an equality
or relational op, and then filter `Fns` by operator name both here and when
adding the non-member function candidates.
The reason is that we need to do these lookups in the context of the template
definition when an equality or relational expression appears within a template,
so we need to look up both the original operator and `<=>` eagerly.
================
Comment at: lib/Sema/SemaOverload.cpp:8936-8938
+ // TODO: We should be able to avoid adding synthesized candidates when LHS
and
+ // RHS have the same type, since the synthesized candidates for <=> should be
+ // the same as the rewritten ones. Note: It's still possible for the result
----------------
Type isn't sufficient to conclude this; you'll need to check the other relevant
properties of LHS and RHS too (at least value kind, but maybe also
bit-field-ness and others?).
================
Comment at: lib/Sema/SemaOverload.cpp:9218-9219
+ // --- F2 is a rewritten candidate ([over.match.oper]) and F1 is not.
+ if (Cand2.getRewrittenKind() && !Cand1.getRewrittenKind())
+ return true;
+ if (Cand1.getRewrittenKind() && Cand2.getRewrittenKind() &&
----------------
You also need to check the reverse condition and return false (the "or if not
that" is ... somehow ... supposed to imply that).
================
Comment at: lib/Sema/SemaOverload.cpp:12502-12535
+ExprResult RewrittenCandidateOverloadResolver::BuildRewrittenCandidate(
+ const OverloadCandidate &Ovl) {
+ Expr *RewrittenArgs[2] = {Args[0], Args[1]};
+ bool IsSynthesized = Ovl.getRewrittenKind() == ROC_Synthesized;
+ if (IsSynthesized)
+ std::swap(RewrittenArgs[0], RewrittenArgs[1]);
+
----------------
This results in an inadequate representation for the rewritten `<=>` form. We
need to retain the "as-written" form of the operator, for multiple reasons
(source fidelity for tooling, expression mangling, pretty-printing, ...).
Generally, Clang's philosophy is to desugar the minimum amount that's necessary
to capture both the syntactic and semantic forms.
A couple of possible representations come to mind here:
1) A specific `Expr` subclass for a rewritten `<=>` operator, which is just a
wrapper around a `(a <=> b) op 0` expression, and knows how to extract the
subexpressions itself, or...
2) A general "rewritten operator wrapper" `Expr` subclass, which holds its
operands and a rewritten expression, and for which the rewritten expression
refers to its operands via `OpaqueValueExpr`s. We already have such a class
(`PseudoObjectExpr`).
The former is a smaller and simpler representation, but will likely be more
work to implement.
================
Comment at: lib/Sema/SemaOverload.cpp:12537-12565
+/// Rewritten candidates have been added but not checked for validity. They
+/// could still be non-viable if:
+/// (A) The rewritten call (x <=> y) is a builtin, but it will be ill-formed
+/// when built (for example it has narrowing conversions).
+/// (B) The expression (x <=> y) @ 0 is ill-formed for the result of (x <=>
y).
+///
+/// If either is the case, this function should be considered non-viable and
----------------
Hmm, this is an interesting approach. It's quite a divergence from our usual
approach, which is to perform the viability checks first, before partial
ordering, even if we could eliminate some candidates based on partial ordering.
And it's observable -- it will result in fewer template instantiations being
performed, which will mean we do not diagnose some ill-formed (no diagnostic
required) situations which would otherwise fail due to an error in a
non-immediate context.
Can you say something about why you chose this approach instead of doing the
lookup for operator `@` early and marking the candidate non-viable if the
lookup / overload resolution for it fails?
(I'm also vaguely wondering whether we should be pushing back on the C++
committee for making viability of the rewritten candidate depend on validity of
the `@` expression, not just the `<=>` expression.)
================
Comment at: lib/Sema/SemaOverload.cpp:12570-12572
+ case OR_Deleted:
+ // FIXME(EricWF): If we've found a deleted rewritten operator, it's
+ // possible we should have never considered it a viable candidate.
----------------
This is a really interesting situation. What does it even mean to consider the
`(a <=> b) @ c` expression if lookup for `<=>` finds a deleted function? (It
would be novel for the return type of a deleted function to be meaningful, and
it would be novel to exclude an overload candidate because it's deleted.) And
what if lookup for `@` finds a deleted function?
I think this is good reason to go back to CWG and suggest that we don't check
the `@` expression at all until after overload resolution.
================
Comment at: lib/Sema/SemaOverload.cpp:12608-12609
+ };
+ // Sort the candidate functions based on their partial ordering,
+ // and find the first N functions which rank equally.
+ std::sort(Overloads.begin(), Overloads.end(), CmpOverloads);
----------------
This is not correct, because it turns out that `isBetterOverloadCandidate` is
not a strict weak order. It's not any kind of order at all, actually, since it
lacks the transitivity property. I don't know whether you can do better than
marking your chosen candidate non-viable and rerunning the candidate selection
algorithm if your presumed-best candidate turns out to be non-viable.
================
Comment at: lib/Sema/SemaOverload.cpp:12634
+ }
+ // If none of the rewritten
+ if (NumViableCandidates > 1)
----------------
I am a vampire and
Repository:
rC Clang
https://reviews.llvm.org/D45680
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
