[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53605: [AST] Pack PredefinedExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added a reviewer: rjmccall.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.
riccibruno added a dependency: D53604: [AST] Widen the bit-fields of Stmt to 8 
bytes.

This patch does 3 things:

1. Move PredefinedExpr below StringLiteral so that it can use its definition.
2. Move the location and the IdentType into the newly available space of the 
bit-fields of Stmt.
3. Only store the function name when needed.


Repository:
  rC Clang

https://reviews.llvm.org/D53605

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h
  lib/AST/ASTImporter.cpp
  lib/AST/Expr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp

Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -388,9 +388,13 @@
 
 void ASTStmtWriter::VisitPredefinedExpr(PredefinedExpr *E) {
   VisitExpr(E);
-  Record.AddSourceLocation(E->getLocation());
+
+  bool HasFunctionName = !!E->getFunctionName();
+  Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentType()); // FIXME: stable encoding
-  Record.AddStmt(E->getFunctionName());
+  Record.AddSourceLocation(E->getLocation());
+  if (HasFunctionName)
+Record.AddStmt(E->getFunctionName());
   Code = serialization::EXPR_PREDEFINED;
 }
 
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -496,9 +496,12 @@
 
 void ASTStmtReader::VisitPredefinedExpr(PredefinedExpr *E) {
   VisitExpr(E);
+  bool HasFunctionName = Record.readInt();
+  E->PredefinedExprBits.HasFnName = HasFunctionName;
+  E->PredefinedExprBits.Type = Record.readInt();
   E->setLocation(ReadSourceLocation());
-  E->Type = (PredefinedExpr::IdentType)Record.readInt();
-  E->FnName = cast_or_null(Record.readSubExpr());
+  if (HasFunctionName)
+E->setFunctionName(cast(Record.readSubExpr()));
 }
 
 void ASTStmtReader::VisitDeclRefExpr(DeclRefExpr *E) {
@@ -2334,12 +2337,14 @@
   break;
 
 case STMT_CAPTURED:
-  S = CapturedStmt::CreateDeserialized(Context,
-   Record[ASTStmtReader::NumStmtFields]);
+  S = CapturedStmt::CreateDeserialized(
+  Context, Record[ASTStmtReader::NumStmtFields]);
   break;
 
 case EXPR_PREDEFINED:
-  S = new (Context) PredefinedExpr(Empty);
+  S = PredefinedExpr::CreateEmpty(
+  Context,
+  /*HasFunctionName*/ Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_DECL_REF:
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -3083,7 +3083,7 @@
 }
   }
 
-  return new (Context) PredefinedExpr(Loc, ResTy, IT, SL);
+  return PredefinedExpr::Create(Context, Loc, ResTy, IT, SL);
 }
 
 ExprResult Sema::ActOnPredefinedExpr(SourceLocation Loc, tok::TokenKind Kind) {
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -463,11 +463,36 @@
 : Expr(PredefinedExprClass, FNTy, VK_LValue, OK_Ordinary,
FNTy->isDependentType(), FNTy->isDependentType(),
FNTy->isInstantiationDependentType(),
-   /*ContainsUnexpandedParameterPack=*/false),
-  Loc(L), Type(IT), FnName(SL) {}
-
-StringLiteral *PredefinedExpr::getFunctionName() {
-  return cast_or_null(FnName);
+   /*ContainsUnexpandedParameterPack=*/false) {
+  PredefinedExprBits.Type = IT;
+  assert((getIdentType() == IT) &&
+ "IdentType do not fit in PredefinedExprBitfields!");
+  bool HasFunctionName = !!SL;
+  PredefinedExprBits.HasFnName = HasFunctionName;
+  PredefinedExprBits.Loc = L;
+  if (HasFunctionName)
+setFunctionName(SL);
+}
+
+PredefinedExpr::PredefinedExpr(EmptyShell Empty, bool HasFunctionName)
+: Expr(PredefinedExprClass, Empty) {
+  PredefinedExprBits.HasFnName = HasFunctionName;
+}
+
+PredefinedExpr *PredefinedExpr::Create(const ASTContext &Ctx, SourceLocation L,
+   QualType FNTy, IdentType IT,
+   StringLiteral *SL) {
+  bool HasFunctionName = !!SL;
+  void *Mem = Ctx.Allocate(totalSizeToAlloc(HasFunctionName),
+   alignof(PredefinedExpr));
+  return new (Mem) PredefinedExpr(L, FNTy, IT, SL);
+}
+
+PredefinedExpr *PredefinedExpr::CreateEmpty(const ASTContext &Ctx,
+bool HasFunctionName) {
+  void *Mem = Ctx.Allocate(totalSizeToAlloc(HasFunctionName),
+   alignof(PredefinedExpr));
+  return new (Mem) PredefinedExpr(EmptyShell(), HasFunctionName);
 }
 
 StringRef PredefinedExpr::getIdentTypeName(PredefinedExpr::IdentType IT) {
I