[PATCH] D53607: [AST] Only store the needed data in IfStmt.

2018-10-27 Thread Phabricator via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC345464: [AST] Only store the needed data in IfStmt (authored 
by brunoricci, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D53607

Files:
  include/clang/AST/Stmt.h
  lib/AST/ASTDumper.cpp
  lib/AST/ASTImporter.cpp
  lib/AST/Stmt.cpp
  lib/Analysis/BodyFarm.cpp
  lib/Sema/SemaStmt.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/Import/if-stmt/test.cpp
  test/Misc/ast-dump-invalid.cpp

Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -151,11 +151,25 @@
   };
 
   class IfStmtBitfields {
+friend class ASTStmtReader;
 friend class IfStmt;
 
 unsigned : NumStmtBits;
 
+/// True if this if statement is a constexpr if.
 unsigned IsConstexpr : 1;
+
+/// True if this if statement has storage for an else statement.
+unsigned HasElse : 1;
+
+/// True if this if statement has storage for a variable declaration.
+unsigned HasVar : 1;
+
+/// True if this if statement has storage for an init statement.
+unsigned HasInit : 1;
+
+/// The location of the "if".
+SourceLocation IfLoc;
   };
 
   class SwitchStmtBitfields {
@@ -1100,21 +1114,117 @@
 };
 
 /// IfStmt - This represents an if/then/else.
-class IfStmt : public Stmt {
-  enum { INIT, VAR, COND, THEN, ELSE, END_EXPR };
-  Stmt* SubExprs[END_EXPR];
+class IfStmt final
+: public Stmt,
+  private llvm::TrailingObjects {
+  friend TrailingObjects;
 
-  SourceLocation IfLoc;
-  SourceLocation ElseLoc;
+  // IfStmt is followed by several trailing objects, some of which optional.
+  // Note that it would be more convenient to put the optional trailing
+  // objects at then end but this would change the order of the children.
+  // The trailing objects are in order:
+  //
+  // * A "Stmt *" for the init statement.
+  //Present if and only if hasInitStorage().
+  //
+  // * A "Stmt *" for the condition variable.
+  //Present if and only if hasVarStorage(). This is in fact a "DeclStmt *".
+  //
+  // * A "Stmt *" for the condition.
+  //Always present. This is in fact a "Expr *".
+  //
+  // * A "Stmt *" for the then statement.
+  //Always present.
+  //
+  // * A "Stmt *" for the else statement.
+  //Present if and only if hasElseStorage().
+  //
+  // * A "SourceLocation" for the location of the "else".
+  //Present if and only if hasElseStorage().
+  enum { InitOffset = 0, ThenOffsetFromCond = 1, ElseOffsetFromCond = 2 };
+  enum { NumMandatoryStmtPtr = 2 };
+
+  unsigned numTrailingObjects(OverloadToken) const {
+return NumMandatoryStmtPtr + hasElseStorage() + hasVarStorage() +
+   hasInitStorage();
+  }
+
+  unsigned numTrailingObjects(OverloadToken) const {
+return hasElseStorage();
+  }
+
+  unsigned initOffset() const { return InitOffset; }
+  unsigned varOffset() const { return InitOffset + hasInitStorage(); }
+  unsigned condOffset() const {
+return InitOffset + hasInitStorage() + hasVarStorage();
+  }
+  unsigned thenOffset() const { return condOffset() + ThenOffsetFromCond; }
+  unsigned elseOffset() const { return condOffset() + ElseOffsetFromCond; }
+
+  /// Build an if/then/else statement.
+  IfStmt(const ASTContext &Ctx, SourceLocation IL, bool IsConstexpr, Stmt *Init,
+ VarDecl *Var, Expr *Cond, Stmt *Then, SourceLocation EL, Stmt *Else);
+
+  /// Build an empty if/then/else statement.
+  explicit IfStmt(EmptyShell Empty, bool HasElse, bool HasVar, bool HasInit);
+
+public:
+  /// Create an IfStmt.
+  static IfStmt *Create(const ASTContext &Ctx, SourceLocation IL,
+bool IsConstexpr, Stmt *Init, VarDecl *Var, Expr *Cond,
+Stmt *Then, SourceLocation EL = SourceLocation(),
+Stmt *Else = nullptr);
+
+  /// Create an empty IfStmt optionally with storage for an else statement,
+  /// condition variable and init expression.
+  static IfStmt *CreateEmpty(const ASTContext &Ctx, bool HasElse, bool HasVar,
+ bool HasInit);
+
+  /// True if this IfStmt has the storage for an init statement.
+  bool hasInitStorage() const { return IfStmtBits.HasInit; }
 
-public:
-  IfStmt(const ASTContext &C, SourceLocation IL,
- bool IsConstexpr, Stmt *init, VarDecl *var, Expr *cond,
- Stmt *then, SourceLocation EL = SourceLocation(),
- Stmt *elsev = nullptr);
+  /// True if this IfStmt has storage for a variable declaration.
+  bool hasVarStorage() const { return IfStmtBits.HasVar; }
+
+  /// True if this IfStmt has storage for an else statement.
+  bool hasElseStorage() const { return IfStmtBits.HasElse; }
 
-  /// Build an empty if/then/else statement
-  explicit IfStmt(EmptyShell Empty) : Stmt(IfStmtClass, Empty) {}
+  Expr *getCond() {
+return reinterpret_cast(getTrailingObjects()[condO

[PATCH] D53607: [AST] Only store the needed data in IfStmt.

2018-10-26 Thread Bruno Ricci via Phabricator via cfe-commits
riccibruno updated this revision to Diff 171332.
riccibruno added a reviewer: rsmith.
riccibruno added a comment.

Add a flag to the output of -ast-dump indicating
which sub-statement `IfStmt` is storing. This removes
the ambiguity in the output of -ast-dump and addresses
rsmith's comment in https://reviews.llvm.org/D53717.


Repository:
  rC Clang

https://reviews.llvm.org/D53607

Files:
  include/clang/AST/Stmt.h
  lib/AST/ASTDumper.cpp
  lib/AST/ASTImporter.cpp
  lib/AST/Stmt.cpp
  lib/Analysis/BodyFarm.cpp
  lib/Sema/SemaStmt.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/Import/if-stmt/test.cpp
  test/Misc/ast-dump-invalid.cpp

Index: test/Misc/ast-dump-invalid.cpp
===
--- test/Misc/ast-dump-invalid.cpp
+++ test/Misc/ast-dump-invalid.cpp
@@ -33,8 +33,6 @@
 // CHECK-NEXT:   |-ParmVarDecl
 // CHECK-NEXT:   `-CompoundStmt
 // CHECK-NEXT: `-IfStmt {{.*}} 
-// CHECK-NEXT:   |-<<>>
-// CHECK-NEXT:   |-<<>>
 // CHECK-NEXT:   |-OpaqueValueExpr {{.*}} <> 'bool'
 // CHECK-NEXT:   |-ReturnStmt {{.*}} 
 // CHECK-NEXT:   | `-IntegerLiteral {{.*}}  'int' 4
@@ -50,15 +48,15 @@
 { return 45; }
 }
 // CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}} TestInvalidFunctionDecl
-// CHECK-NEXT: |-CXXRecordDecl {{.*}}  line:46:8 struct Str definition
+// CHECK-NEXT: |-CXXRecordDecl {{.*}}  line:44:8 struct Str definition
 // CHECK:  | |-CXXRecordDecl {{.*}}  col:8 implicit struct Str
-// CHECK-NEXT: | `-CXXMethodDecl {{.*}}  col:11 invalid foo1 'double (double, int)'
+// CHECK-NEXT: | `-CXXMethodDecl {{.*}}  col:11 invalid foo1 'double (double, int)'
 // CHECK-NEXT: |   |-ParmVarDecl {{.*}}  col:22 'double'
 // CHECK-NEXT: |   `-ParmVarDecl {{.*}} > col:36 invalid 'int'
-// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}}  line:49:13 invalid foo1 'double (double, int)'
+// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}}  line:47:13 invalid foo1 'double (double, int)'
 // CHECK-NEXT:   |-ParmVarDecl {{.*}}  col:24 'double'
 // CHECK-NEXT:   |-ParmVarDecl {{.*}} > col:38 invalid 'int'
-// CHECK-NEXT:   `-CompoundStmt {{.*}} 
+// CHECK-NEXT:   `-CompoundStmt {{.*}} 
 // CHECK-NEXT: `-ReturnStmt {{.*}} 
 // CHECK-NEXT:   `-ImplicitCastExpr {{.*}}  'double' 
 // CHECK-NEXT: `-IntegerLiteral {{.*}}  'int' 45
Index: test/Import/if-stmt/test.cpp
===
--- test/Import/if-stmt/test.cpp
+++ test/Import/if-stmt/test.cpp
@@ -1,41 +1,30 @@
 // RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | FileCheck %s
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
 // CHECK-NEXT: DeclStmt
 // CHECK-NEXT: VarDecl
 // CHECK-NEXT: IntegerLiteral
 // CHECK-NEXT: ImplicitCastExpr
 // CHECK-NEXT: ImplicitCastExpr
 // CHECK-NEXT: DeclRefExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
 
 // CHECK: IfStmt
 // CHECK-NEXT: DeclStmt
 // CHECK-NEXT: VarDecl
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
 // CHECK-NEXT: ReturnStmt
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT: ReturnStmt
Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -128,14 +128,29 @@
 
 void ASTStmtWriter::VisitIfStmt(IfStmt *S) {
   VisitStmt(S);
+
+  bool HasElse = S->getElse() != nullptr;
+  bool HasVar = S->getConditionVariableDeclStmt() != nullptr;
+  bool HasInit = S->getInit() != nullptr;
+
   Record.push_back(S->isConstexpr());
-  Record.AddStmt(S->getInit());
-  Record.AddDeclRef(S->getConditionVariable());
+  Record.push_back(HasElse);
+  Record.push_back(HasVar);
+  Record.push_back(HasInit);
+
   Record.AddStmt(S->getCond());
   Record.AddStmt(S->getThen());
-  Record.AddStmt(S->getElse());
+  if (HasElse)
+Record.AddStmt(S->getElse());
+  if (HasVar)
+Record.AddDeclRef(S->getConditionVariable());
+  if (HasInit)
+Record.AddStmt(S->getInit());
+
   Record.AddSourceLocation(S->getIfLoc());
-  Record.AddSourceLocation(S->getElseLoc());
+  if (HasElse)
+Record.AddSourceLocation(S->getElseLoc());
+
   Code = serialization::STMT_IF;
 }
 
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -215,14 +215,24 @@
 
 void ASTStmtReader::VisitIfStmt(IfStmt *S) {
   VisitStmt(S);
+
   S->setConstexpr(Record.readInt());
-  S->setInit(Record.readSubStmt());
-  S->setConditionVariable(R

[PATCH] D53607: [AST] Only store the needed data in IfStmt.

2018-10-25 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D53607



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


[PATCH] D53607: [AST] Only store the needed data in IfStmt.

2018-10-25 Thread Bruno Ricci via Phabricator via cfe-commits
riccibruno updated this revision to Diff 171121.
riccibruno edited the summary of this revision.
riccibruno added a comment.

Reworked so that the order of the children is kept the same.


Repository:
  rC Clang

https://reviews.llvm.org/D53607

Files:
  include/clang/AST/Stmt.h
  lib/AST/ASTImporter.cpp
  lib/AST/Stmt.cpp
  lib/Analysis/BodyFarm.cpp
  lib/Sema/SemaStmt.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/Import/if-stmt/test.cpp
  test/Misc/ast-dump-invalid.cpp

Index: test/Misc/ast-dump-invalid.cpp
===
--- test/Misc/ast-dump-invalid.cpp
+++ test/Misc/ast-dump-invalid.cpp
@@ -33,8 +33,6 @@
 // CHECK-NEXT:   |-ParmVarDecl
 // CHECK-NEXT:   `-CompoundStmt
 // CHECK-NEXT: `-IfStmt {{.*}} 
-// CHECK-NEXT:   |-<<>>
-// CHECK-NEXT:   |-<<>>
 // CHECK-NEXT:   |-OpaqueValueExpr {{.*}} <> 'bool'
 // CHECK-NEXT:   |-ReturnStmt {{.*}} 
 // CHECK-NEXT:   | `-IntegerLiteral {{.*}}  'int' 4
@@ -50,15 +48,15 @@
 { return 45; }
 }
 // CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}} TestInvalidFunctionDecl
-// CHECK-NEXT: |-CXXRecordDecl {{.*}}  line:46:8 struct Str definition
+// CHECK-NEXT: |-CXXRecordDecl {{.*}}  line:44:8 struct Str definition
 // CHECK:  | |-CXXRecordDecl {{.*}}  col:8 implicit struct Str
-// CHECK-NEXT: | `-CXXMethodDecl {{.*}}  col:11 invalid foo1 'double (double, int)'
+// CHECK-NEXT: | `-CXXMethodDecl {{.*}}  col:11 invalid foo1 'double (double, int)'
 // CHECK-NEXT: |   |-ParmVarDecl {{.*}}  col:22 'double'
 // CHECK-NEXT: |   `-ParmVarDecl {{.*}} > col:36 invalid 'int'
-// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}}  line:49:13 invalid foo1 'double (double, int)'
+// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}}  line:47:13 invalid foo1 'double (double, int)'
 // CHECK-NEXT:   |-ParmVarDecl {{.*}}  col:24 'double'
 // CHECK-NEXT:   |-ParmVarDecl {{.*}} > col:38 invalid 'int'
-// CHECK-NEXT:   `-CompoundStmt {{.*}} 
+// CHECK-NEXT:   `-CompoundStmt {{.*}} 
 // CHECK-NEXT: `-ReturnStmt {{.*}} 
 // CHECK-NEXT:   `-ImplicitCastExpr {{.*}}  'double' 
 // CHECK-NEXT: `-IntegerLiteral {{.*}}  'int' 45
Index: test/Import/if-stmt/test.cpp
===
--- test/Import/if-stmt/test.cpp
+++ test/Import/if-stmt/test.cpp
@@ -1,41 +1,30 @@
 // RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | FileCheck %s
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
 // CHECK-NEXT: DeclStmt
 // CHECK-NEXT: VarDecl
 // CHECK-NEXT: IntegerLiteral
 // CHECK-NEXT: ImplicitCastExpr
 // CHECK-NEXT: ImplicitCastExpr
 // CHECK-NEXT: DeclRefExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
 
 // CHECK: IfStmt
 // CHECK-NEXT: DeclStmt
 // CHECK-NEXT: VarDecl
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
 // CHECK-NEXT: ReturnStmt
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT: ReturnStmt
Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -128,14 +128,29 @@
 
 void ASTStmtWriter::VisitIfStmt(IfStmt *S) {
   VisitStmt(S);
+
+  bool HasElse = !!S->getElse();
+  bool HasVar = !!S->getConditionVariableDeclStmt();
+  bool HasInit = !!S->getInit();
+
   Record.push_back(S->isConstexpr());
-  Record.AddStmt(S->getInit());
-  Record.AddDeclRef(S->getConditionVariable());
+  Record.push_back(HasElse);
+  Record.push_back(HasVar);
+  Record.push_back(HasInit);
+
   Record.AddStmt(S->getCond());
   Record.AddStmt(S->getThen());
-  Record.AddStmt(S->getElse());
+  if (HasElse)
+Record.AddStmt(S->getElse());
+  if (HasVar)
+Record.AddDeclRef(S->getConditionVariable());
+  if (HasInit)
+Record.AddStmt(S->getInit());
+
   Record.AddSourceLocation(S->getIfLoc());
-  Record.AddSourceLocation(S->getElseLoc());
+  if (HasElse)
+Record.AddSourceLocation(S->getElseLoc());
+
   Code = serialization::STMT_IF;
 }
 
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -215,14 +215,24 @@
 
 void ASTStmtReader::VisitIfStmt(IfStmt *S) {
   VisitStmt(S);
+
   S->setConstexpr(Record.readInt());
-  S->setInit(Record.readSubStmt());
-  S->setConditionVariable(Record.getContext(), ReadDeclAs());
+  bool HasElse = Record.readInt();
+  bool HasVar = Record.readInt();
+  bool HasInit = Record.readInt();
+
   S->setCond(Record.readSubExpr());
   S->set

[PATCH] D53607: [AST] Only store the needed data in IfStmt.

2018-10-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment.

Yeah, I agree that changing child order is problematic.


Repository:
  rC Clang

https://reviews.llvm.org/D53607



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


[PATCH] D53607: [AST] Only store the needed data in IfStmt.

2018-10-24 Thread Bruno Ricci via Phabricator via cfe-commits
riccibruno added a comment.

To add on the issue of changing the order of the children. The more I think 
about
it the less it seems to be a good idea. Every test passes but I strongly suspect
that a lot of code subtly rely on this.


Repository:
  rC Clang

https://reviews.llvm.org/D53607



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


[PATCH] D53607: [AST] Only store the needed data in IfStmt.

2018-10-23 Thread Bruno Ricci via Phabricator via cfe-commits
riccibruno created this revision.
riccibruno added a reviewer: rjmccall.
riccibruno added a project: clang.
Herald added subscribers: cfe-commits, javed.absar.
riccibruno added a dependency: D53605: [AST] Pack PredefinedExpr.

Only store the needed data in `IfStmt`. The various `Stmt *` are put in
a trailing array with the commonly used `Stmt *` first (like the condition
and the then statement) and the less commonly used `Stmt *` last.
This cuts the size of `IfStmt` by up to 3 pointers + 1 `SourceLocation`.

One point which must be discussed is that this changes the order of the
children. This is strictly speaking not needed (we could just compute the
appropriate index into the trailing array) but this allows the common data
to be at a constant offset into the trailing array. This saves us from doing
bit manipulations and arithmetic each time the commonly used data is accessed.

I believe the C API is unaffected since it do not rely on calling `children` on 
`IfStmt`.
This also makes modifying an `IfStmt` after creation harder since the necessary
storage might not exist.


Repository:
  rC Clang

https://reviews.llvm.org/D53607

Files:
  include/clang/AST/Stmt.h
  lib/AST/ASTImporter.cpp
  lib/AST/Stmt.cpp
  lib/Analysis/BodyFarm.cpp
  lib/Sema/SemaStmt.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/Import/if-stmt/test.cpp
  test/Misc/ast-dump-invalid.cpp

Index: test/Misc/ast-dump-invalid.cpp
===
--- test/Misc/ast-dump-invalid.cpp
+++ test/Misc/ast-dump-invalid.cpp
@@ -33,8 +33,6 @@
 // CHECK-NEXT:   |-ParmVarDecl
 // CHECK-NEXT:   `-CompoundStmt
 // CHECK-NEXT: `-IfStmt {{.*}} 
-// CHECK-NEXT:   |-<<>>
-// CHECK-NEXT:   |-<<>>
 // CHECK-NEXT:   |-OpaqueValueExpr {{.*}} <> 'bool'
 // CHECK-NEXT:   |-ReturnStmt {{.*}} 
 // CHECK-NEXT:   | `-IntegerLiteral {{.*}}  'int' 4
@@ -50,15 +48,15 @@
 { return 45; }
 }
 // CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}} TestInvalidFunctionDecl
-// CHECK-NEXT: |-CXXRecordDecl {{.*}}  line:46:8 struct Str definition
+// CHECK-NEXT: |-CXXRecordDecl {{.*}}  line:44:8 struct Str definition
 // CHECK:  | |-CXXRecordDecl {{.*}}  col:8 implicit struct Str
-// CHECK-NEXT: | `-CXXMethodDecl {{.*}}  col:11 invalid foo1 'double (double, int)'
+// CHECK-NEXT: | `-CXXMethodDecl {{.*}}  col:11 invalid foo1 'double (double, int)'
 // CHECK-NEXT: |   |-ParmVarDecl {{.*}}  col:22 'double'
 // CHECK-NEXT: |   `-ParmVarDecl {{.*}} > col:36 invalid 'int'
-// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}}  line:49:13 invalid foo1 'double (double, int)'
+// CHECK-NEXT: `-CXXMethodDecl {{.*}} parent {{.*}}  line:47:13 invalid foo1 'double (double, int)'
 // CHECK-NEXT:   |-ParmVarDecl {{.*}}  col:24 'double'
 // CHECK-NEXT:   |-ParmVarDecl {{.*}} > col:38 invalid 'int'
-// CHECK-NEXT:   `-CompoundStmt {{.*}} 
+// CHECK-NEXT:   `-CompoundStmt {{.*}} 
 // CHECK-NEXT: `-ReturnStmt {{.*}} 
 // CHECK-NEXT:   `-ImplicitCastExpr {{.*}}  'double' 
 // CHECK-NEXT: `-IntegerLiteral {{.*}}  'int' 45
Index: test/Import/if-stmt/test.cpp
===
--- test/Import/if-stmt/test.cpp
+++ test/Import/if-stmt/test.cpp
@@ -1,41 +1,30 @@
 // RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | FileCheck %s
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: DeclStmt
-// CHECK-NEXT: VarDecl
-// CHECK-NEXT: IntegerLiteral
 // CHECK-NEXT: ImplicitCastExpr
 // CHECK-NEXT: ImplicitCastExpr
 // CHECK-NEXT: DeclRefExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
-
-// CHECK: IfStmt
 // CHECK-NEXT: DeclStmt
 // CHECK-NEXT: VarDecl
-// CHECK-NEXT: <>
+// CHECK-NEXT: IntegerLiteral
+
+// CHECK: IfStmt
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
-// CHECK-NEXT: <>
+// CHECK-NEXT: DeclStmt
+// CHECK-NEXT: VarDecl
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: ReturnStmt
 // CHECK-NEXT: ReturnStmt
 
 // CHECK: IfStmt
-// CHECK-NEXT: <>
-// CHECK-NEXT: <>
 // CHECK-NEXT: CXXBoolLiteralExpr
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT: ReturnStmt
Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -128,14 +128,29 @@
 
 void ASTStmtWriter::VisitIfStmt(IfStmt *S) {
   VisitStmt(S);
+
+  bool HasElse = !!S->getElse();
+  bool HasVar = !!S->getConditionVariableDeclStmt();
+  bool HasInit = !!S->getInit();
+
   Record.push_back(S->isConstexpr());
-  Record.AddStmt(S->getInit());
-  Record.AddDeclRef(S->getConditionVariable());
+  Record.push_back(HasElse);
+  Record.push_back(HasVar);
+  Record.push_back(HasInit);
+
   Record.AddStmt(S->getCond());