[PATCH] D53475: Create ConstantExpr class

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

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks, looks good. I don't mind if you address the comments below before this commit or in a separate commit.



================
Comment at: include/clang/AST/Expr.h:898
+class ConstantExpr : public FullExpr {
+  Stmt *Val;
+public:
----------------
I think it'd be cleaner and simpler to move this field into the base class, merging it with the corresponding member of `ExprWithCleanups`.


================
Comment at: include/clang/AST/StmtDataCollectors.td:17-23
+//--- Wrappers -----------------------------------------------------------//
+class ConstantExpr {
+  code Code = [{
+    addData(S->getType());
+  }];
+}
+
----------------
Do we need this? I think we probably don't:
a) we don't have corresponding code for `ExprWithCleanups`
b) we already add the type via the `Expr` handler
c) the type of `ConstantExpr` doesn't actually add any new information because it's always the same as the type of the wrapped expression


================
Comment at: lib/AST/StmtProfile.cpp:1001
+  VisitExpr(S);
+  VisitExpr(S->getSubExpr());
+}
----------------
void wrote:
> rsmith wrote:
> > This is unnecessary: this visitor visits the children of the node for you.
> If I don't have this then the link fails because it cannot find this.
No worries: I think this comment was against a previous version where you were also explicitly visiting the substatement of `S`.


Repository:
  rC Clang

https://reviews.llvm.org/D53475