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

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

rsmith added a comment.

Can you split this patch up a bit? There's changes here for the `<=>` rewriting, but also for supporting `operator<=>(...) = default`, and some constant evaluation and code generation changes too. It's a lot easier to review more directed, smaller patches.



================
Comment at: include/clang/AST/ExprCXX.h:4212
 
+class CXXRewrittenOperator : public Expr {
+  friend class ASTReader;
----------------
This should have a name ending `Expr`.


================
Comment at: include/clang/AST/ExprCXX.h:4230-4231
+public:
+  // FIXME(EricWF): Figure out if this will even be built for dependent
+  // expressions.
+  CXXRewrittenOperator(RewrittenOperatorKind Kind, Expr *Underlying,
----------------
It probably won't ever be type-dependent, but could absolutely be value-dependent, instantiation-dependent, or contain an unexpanded parameter pack.


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


================
Comment at: lib/AST/ItaniumMangle.cpp:3555
+  case Expr::CXXRewrittenOperatorClass:
+    // FIXME(EricWF): Is this correct?
+    mangleExpression(cast<CXXRewrittenOperator>(E)->getRewrittenExpr(), Arity);
----------------
No, the expression should mangle as-written.


https://reviews.llvm.org/D45680