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