[AST] Make NullStmt final and give it factory functions

[hamzasood]

Summary

I've submitted a patch for contracts (review linked to this) that adds trailing objects to NullStmt. This patch contains the changes to make that possible.

[steveire]

This looks like a NFC change.

Given that the next patch moves these methods out of line, you might consider introducing them out of line here (and moving the constructors out of line). That would make the next patch easier to review because the addition of assertion handling would be move visible.

You could also state in the commit message why this is necessary to make your contracts possible. From looking at the follow-up patch, it seems that you want to centralize creation so that it doesn't have to be duplicated in the follow-up patch.

LGTM, but I'm new here, so you may want to wait for another reviewer if you wish.

[hamzasood]

Thanks. I'm not planning to move forward with this until the contracts patch is ready (it might change enough to not even require this); that should leave plenty of time for others to take a look.

include/clang/AST/Stmt.h

public:
 
   reverse_decl_iterator decl_rend() {
     return reverse_decl_iterator(decl_begin());
   }
 };
 
 /// NullStmt - This is the null statement ";": C99 6.8.3p3.
 ///
-class NullStmt : public Stmt {
+class NullStmt final : public Stmt {
   SourceLocation SemiLoc;
 
   /// True if the null statement was preceded by an empty macro, e.g:
   /// @code
   ///   #define CALL(x)
   ///   CALL(0);
   /// @endcode
-  bool HasLeadingEmptyMacro = false;
+  bool HasLeadingEmptyMacro;
+
+  NullStmt(SourceLocation L, bool hasLeadingEmptyMacro)
+      : Stmt(NullStmtClass), SemiLoc(L),
+        HasLeadingEmptyMacro(hasLeadingEmptyMacro) {}
+
+  explicit NullStmt(EmptyShell Empty)
+      : Stmt(NullStmtClass, Empty), HasLeadingEmptyMacro(false) {}
 
 public:
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
 
-  NullStmt(SourceLocation L, bool hasLeadingEmptyMacro = false)
-      : Stmt(NullStmtClass), SemiLoc(L),
-        HasLeadingEmptyMacro(hasLeadingEmptyMacro) {}
+  static NullStmt *Create(const ASTContext &C, SourceLocation L,
+                          bool hasLeadingEmptyMacro = false) {
+    return new (C) NullStmt(L, hasLeadingEmptyMacro);
+  }
 
   /// Build an empty null statement.
-  explicit NullStmt(EmptyShell Empty) : Stmt(NullStmtClass, Empty) {}
+  static NullStmt *CreateEmpty(const ASTContext &C) {
+    return new (C) NullStmt(EmptyShell{});
+  }
 
   SourceLocation getSemiLoc() const { return SemiLoc; }
   void setSemiLoc(SourceLocation L) { SemiLoc = L; }
 
   bool hasLeadingEmptyMacro() const { return HasLeadingEmptyMacro; }
 
   LLVM_ATTRIBUTE_DEPRECATED(SourceLocation getLocStart() const LLVM_READONLY,
                             "Use getBeginLoc instead") {

lib/AST/ASTImporter.cpp

Stmt *ASTNodeImporter::VisitDeclStmt(DeclStmt *S) {
   }
   SourceLocation ToStartLoc = Importer.Import(S->getBeginLoc());
   SourceLocation ToEndLoc = Importer.Import(S->getEndLoc());
   return new (Importer.getToContext()) DeclStmt(ToDG, ToStartLoc, ToEndLoc);
 }
 
 Stmt *ASTNodeImporter::VisitNullStmt(NullStmt *S) {
   SourceLocation ToSemiLoc = Importer.Import(S->getSemiLoc());
-  return new (Importer.getToContext()) NullStmt(ToSemiLoc,
-                                                S->hasLeadingEmptyMacro());
+  return NullStmt::Create(Importer.getToContext(), ToSemiLoc,
 }
 
 Stmt *ASTNodeImporter::VisitCompoundStmt(CompoundStmt *S) {
   SmallVector<Stmt *, 8> ToStmts(S->size());
 
   if (ImportContainerChecked(S->body(), ToStmts))
     return nullptr;
 

lib/Sema/SemaStmt.cpp

 
 StmtResult Sema::ActOnExprStmtError() {
   DiscardCleanupsInEvaluationContext();
   return StmtError();
 }
 
 StmtResult Sema::ActOnNullStmt(SourceLocation SemiLoc,
                                bool HasLeadingEmptyMacro) {
-  return new (Context) NullStmt(SemiLoc, HasLeadingEmptyMacro);
+  return NullStmt::Create(Context, SemiLoc, HasLeadingEmptyMacro);
 }
 
 StmtResult Sema::ActOnDeclStmt(DeclGroupPtrTy dg, SourceLocation StartLoc,
                                SourceLocation EndLoc) {
   DeclGroupRef DG = dg.get();
 
   // If we have an invalid decl, just return an error.
   if (DG.isNull()) return StmtError();

lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformIfStmt(IfStmt *S) {
 
   // Transform the "then" branch.
   StmtResult Then;
   if (!ConstexprConditionValue || *ConstexprConditionValue) {
     Then = getDerived().TransformStmt(S->getThen());
     if (Then.isInvalid())
       return StmtError();
   } else {
-    Then = new (getSema().Context) NullStmt(S->getThen()->getBeginLoc());
+    Then = NullStmt::Create(getSema().Context, S->getThen()->getBeginLoc());
   }
 
   // Transform the "else" branch.
   StmtResult Else;
   if (!ConstexprConditionValue || !*ConstexprConditionValue) {
     Else = getDerived().TransformStmt(S->getElse());
     if (Else.isInvalid())
       return StmtError();
TreeTransform<Derived>::TransformMSDependentExistsStmt(
   CXXScopeSpec SS;
   SS.Adopt(QualifierLoc);
   bool Dependent = false;
   switch (getSema().CheckMicrosoftIfExistsSymbol(/*S=*/nullptr, SS, NameInfo)) {
   case Sema::IER_Exists:
     if (S->isIfExists())
       break;
 
-    return new (getSema().Context) NullStmt(S->getKeywordLoc());
+    return NullStmt::Create(getSema().Context, S->getKeywordLoc());
 
   case Sema::IER_DoesNotExist:
     if (S->isIfNotExists())
       break;
 
-    return new (getSema().Context) NullStmt(S->getKeywordLoc());
+    return NullStmt::Create(getSema().Context, S->getKeywordLoc());
 
   case Sema::IER_Dependent:
     Dependent = true;
     break;
 
   case Sema::IER_Error:
     return StmtError();
   }

lib/Serialization/ASTReaderStmt.cpp

case STMT_REF_PTR:
       S = StmtEntries[Record.readInt()];
       break;
 
     case STMT_NULL_PTR:
       S = nullptr;
       break;
 
     case STMT_NULL:
-      S = new (Context) NullStmt(Empty);
+      S = NullStmt::CreateEmpty(Context);
       break;
 
     case STMT_COMPOUND:
       S = CompoundStmt::CreateEmpty(
           Context, /*NumStmts=*/Record[ASTStmtReader::NumStmtFields]);
       break;
 
     case STMT_CASE: