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

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

rsmith 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 {
----------------
I don't think you need this.


================
Comment at: include/clang/AST/ExprCXX.h:4294-4296
+  Expr *getLHS() const { return getLHSExpr(getRewrittenExpr()); }
+  Expr *getRHS() const { return getRHSExpr(getRewrittenExpr()); }
+  Opcode getOpcode() const { return getOpcodeFromExpr(getRewrittenExpr()); }
----------------
rsmith wrote:
> I find the interface exposed by this class a little confusing. On the one hand, it provides the original expression as an `Expr*`, and on the other hand it hard-codes knowledge about the form of that expression and exposes accessors into that original form. The upshot is that we pay the cost of a general mechanism (that can cope with any form of original expression) but don't get any benefit from that (because the class hard-codes non-generality).
> 
> This also generates a distinctly non-tree-shaped AST; clients naively walking recursively over expressions and their subexpressions will see the same`Expr*` multiple times and potentially end up performing an exponential-time tree walk.
> 
> I think there are (at least) two reasonable approaches here:
> 
> 1) Use `OpaqueValueExpr`s to handle the repeated AST nodes, and track an original expression, a rewritten expression, and probably an original LHS expr and an original RHS expr; the original and rewrite would use OVEs to refer indirectly to the LHS and RHS. Remove all hardcoded knowledge of the original and rewrite from this expression node and make it generic for rewritten expression forms.
> 
> 2) Make this node explicitly be specific to spaceship rewrites. Remove the original expression pointer and rename it to something more specific. Keep the accessors that make hardcoded assumptions about the form of the rewritten expression.
> 
> Option 2 looks closer to what this patch is currently doing (you're not using the original expression much).
As noted, if you want this to be a general-purpose "rewritten expression" node, you should use `OpaqueValueExpr`s to track the repeated AST nodes between the original and rewritten expression forms rather than forming a non-tree-shaped AST. However, if you want to take that path, you don't need to add a new `Expr` node for that; that's what `PseudoObjectExpr` is for.


================
Comment at: include/clang/AST/Stmt.h:254
+
+    unsigned Kind : 1;
+  };
----------------
I don't think you need this either.


================
Comment at: include/clang/Sema/Overload.h:938
+    /// exited.
+    struct RewrittenCandidateContextGuard {
+      RewrittenCandidateContextGuard(OverloadCandidateSet &CS)
----------------
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>`.


================
Comment at: include/clang/Sema/Overload.h:81-83
+    ROC_Rewritten,
+    /// Both rewritten and synthesized.
+    ROC_Synthesized
----------------
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?


================
Comment at: include/clang/Serialization/ASTBitCodes.h:1814
+      EXPR_CXX_FOLD,                          // CXXFoldExpr
+      EXPR_CXX_REWRITTEN_OPERATOR,            // CXXRewrittenExpr
 
----------------
Drop the `_OPERATOR` here.


================
Comment at: lib/Sema/SemaOverload.cpp:12542
+  // FIXME(EricWF): This doesn't actually really represent the expression as
+  // written, because it may not result in a to a builtin operator.
+  Expr *Original = new (S.Context)
----------------
> in a *call* to a


================
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);
----------------
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.)


================
Comment at: lib/Sema/TreeTransform.h:11569-11573
+    // Extract the already transformed operands from the rewritten expression.
+    std::pair<Expr *, Expr *> OrigArgs =
+        extractOriginalOperandsFromRewrittenComparison(
+            Rewritten, Op->getOpcode() == BO_Cmp,
+            E->getRewrittenInfo()->CompareBits.IsSynthesized);
----------------
This doesn't seem right to me; `TreeTransform` should be transforming the original expression, not the rewritten one. We shouldn't be assuming we know what kind of transforms the client wants to perform, it's just our job to give them the syntactic pieces and ask for those pieces to be transformed.

(But this is all redundant if you switch to using `PseudoObjectExpr`.)


https://reviews.llvm.org/D45680