[PATCH] D53717: [AST] Only store the needed data in ForStmt.

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

rsmith added a comment.

Do you have evidence that this complexity is worthwhile? (I wouldn't imagine we have very many `ForStmt`s per translation unit, so saving 16 bytes for each of them seems unlikely to matter.)



================
Comment at: include/clang/AST/Stmt.h:1863
+  Expr *getCond() {
+    return reinterpret_cast<Expr *>(getTrailingObjects<Stmt *>()[condOffset()]);
+  }
----------------
Please `static_cast` rather than `reinterpret_cast` throughout. (If the `Stmt` base class weren't at offset 0 within `Expr`, we'd want to apply the adjustment.)


================
Comment at: include/clang/AST/Stmt.h:1871
+  void setCond(Expr *Cond) {
+    getTrailingObjects<Stmt *>()[condOffset()] = reinterpret_cast<Stmt *>(Cond);
+  }
----------------
No need for an explicit cast from `Expr` to base class `Stmt`.


================
Comment at: test/Import/for-stmt/test.cpp:3-8
 // CHECK: ForStmt
 // CHECK-NEXT: <<NULL>>
 // CHECK-NEXT: <<NULL>>
-// CHECK-NEXT: <<NULL>>
-// CHECK-NEXT: <<NULL>>
 // CHECK-NEXT: NullStmt
----------------
This seems like a (minor) problem: `-ast-dump` no longer shows what the things it's dumping mean. Maybe add the has_init / has_var flags to the dump of the `ForStmt`, or add labels to the substatements, or something?


Repository:
  rC Clang

https://reviews.llvm.org/D53717