[PATCH] D45680: [C++2a] Implement operator<=> Part 2: Operator Rewritting and Overload Resolution.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: include/clang/AST/ExprCXX.h:4218-4226
+  struct ComparisonBits {
+    /// Whether this rewritten comparison expression has reverse-order
+    /// parameters.
+    unsigned IsSynthesized : 1;
+  };
+
+  union ExtraRewrittenBits {
----------------
rsmith wrote:
> I don't think you need this.
I guess it's just future proofing, or an attempt to demonstrate how the class may be generalized. I'll remove it for now.


================
Comment at: include/clang/Sema/Overload.h:938
+    /// exited.
+    struct RewrittenCandidateContextGuard {
+      RewrittenCandidateContextGuard(OverloadCandidateSet &CS)
----------------
rsmith wrote:
> EricWF wrote:
> > This is unneeded. As long as the rewritten candidates are added last, restoring the `Functions` member is unneeded. 
> Please instead extend the `Functions` set to be a set of `PointerIntPair<Decl*, 2, RewrittenOperatorCandidateKind>`.
Sure. I'll get that done.

There are two options here:

(A) Keep the guard object. Store the RewrittenOverloadCandidateKind which we're currently adding candidates for, and have `addCandidate` and `isNewCandidate` handle the ROC.

(B) Change all of the `AddFooCandidates` interfaces in Sema to take yet another default parameter, and pass it around everywhere.

Which would you prefer @rsmith?


================
Comment at: include/clang/Sema/Overload.h:81-83
+    ROC_Rewritten,
+    /// Both rewritten and synthesized.
+    ROC_Synthesized
----------------
rsmith wrote:
> These names are not very good. The standard describes both these cases as "rewritten", so `ROC_Rewritten` is ambiguous, and it says "a synthesized candidate with reversed order of parameters", not merely "synthesized" (which would fail to communicate what's going on).
> 
> How about `ROC_AsThreeWayComparison` and `ROC_AsReversedThreeWayComparison` or similar?
Sounds good to me.


================
Comment at: lib/Sema/SemaOverload.cpp:12543-12547
+  Expr *Original = new (S.Context)
+      BinaryOperator(OpaqueValueExpr::Create(S.Context, Args[0]),
+                     OpaqueValueExpr::Create(S.Context, Args[1]), Opc,
+                     Rewritten->getType(), Rewritten->getValueKind(),
+                     Rewritten->getObjectKind(), OpLoc, S.FPFeatures);
----------------
rsmith wrote:
> If you want to model this as a generic (syntactic form, semantic form) pair, the syntactic form needs to preserve enough information that `TreeTransform` on it can recreate the semantic form. That means you need to store a `CXXOperatorCallExpr` to an `UnresolvedLookupExpr` to hold `Fns` if `Fns` is not empty (see the code at the start of `CreateOverloadedBinOp` that deals with the type-dependent case for an example of what you would need to do).
> 
> Though perhaps this is as good a reason as any to give up the idea of a generic rewrite expression and add something specific to a three-way comparison rewrite; the representation needed to fit this into a generic rewrite mechanism is very unwieldy. (If we add any more forms of rewritten operator later, we can consider whether a generalization is worthwhile.)
I think the FIXME comment may be incorrect.

Shouldn't this be sufficient since we never actually build the rewritten expression until the input expressions are no longer type dependent?
Additionally, with this representation `TreeTransform` can transform the semantic form, since (if I'm correct), a rebuild should never re-perform overload resolution to choose how to rewrite the initial expression. (Does that make sense, and is it correct?)



I'll go ahead and start converting to a less-general representation of the rewritten expression.


https://reviews.llvm.org/D45680