[PATCH] D45680: [C++2a] Add operator<=> Rewriting - Early Attempt

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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