[PATCH] D53605: [AST] Pack PredefinedExpr

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/Expr.h:1687
+  // "Stmt *" for the predefined identifier. It is present if and only if
+  // hasFunctionName() is true and is in fact a "StringLiteral *".
+
----------------
"always" would be clearer than "in fact".


================
Comment at: include/clang/AST/Stmt.h:279
+    /// in PredefinedExpr::IdentType.
+    unsigned Type : 4;
+
----------------
Since you're changing this around anyway, please make it a "kind" rather than a "type".  Even if it's just the field name for now, it's progress.


================
Comment at: include/clang/AST/Stmt.h:283
+    /// for the predefined identifier.
+    unsigned HasFnName : 1;
+
----------------
Not sure why this is abbreviated.


================
Comment at: lib/AST/Expr.cpp:470
+         "IdentType do not fit in PredefinedExprBitfields!");
+  bool HasFunctionName = !!SL;
+  PredefinedExprBits.HasFnName = HasFunctionName;
----------------
`!= nullptr` is clearer.


Repository:
  rC Clang

https://reviews.llvm.org/D53605