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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
