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 
+//    is well formed.
No `\brief` here (we use autobrief), and please use `///` for continuation 

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 
+  // 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 

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 <=> 
+/// 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

  rC Clang

cfe-commits mailing list

Reply via email to