Remove BlockStatement::append()

Bug: tint:396
Bug: tint:390
Change-Id: I3b558a8961f9890f24d1aa3d6647ec095e5fe1cb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/35421
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: David Neto <dneto@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

src/reader/spirv/function.cc

@@ -608,7 +608,7 @@ class StructuredTraverser {
   std::unordered_set<uint32_t> visited_;
 };
 
-/// A StatementBuilder for ast::SwitchStatment
+/// A StatementBuilder for ast::SwitchStatement
 /// @see StatementBuilder
 struct SwitchStatementBuilder
     : public Castable<SwitchStatementBuilder, StatementBuilder> {
@@ -730,32 +730,17 @@ FunctionEmitter::StatementBlock::StatementBlock(
       completion_action_(completion_action),
       cases_(cases) {}
 
-FunctionEmitter::StatementBlock::StatementBlock(StatementBlock&& other)
-    : construct_(other.construct_),
-      end_id_(other.end_id_),
-      completion_action_(std::move(other.completion_action_)),
-      statements_(std::move(other.statements_)),
-      cases_(std::move(other.cases_)) {
-  other.statements_.clear();
-}
+FunctionEmitter::StatementBlock::StatementBlock(StatementBlock&& other) =
+    default;
 
-FunctionEmitter::StatementBlock::~StatementBlock() {
-  if (!finalized_) {
-    // Delete builders that have not been built with Finalize()
-    for (auto* statement : statements_) {
-      if (auto* builder = statement->As<StatementBuilder>()) {
-        delete builder;
-      }
-    }
-  }
-}
+FunctionEmitter::StatementBlock::~StatementBlock() = default;
 
 void FunctionEmitter::StatementBlock::Finalize(ast::Module* mod) {
   assert(!finalized_ /* Finalize() must only be called once */);
+
   for (size_t i = 0; i < statements_.size(); i++) {
     if (auto* builder = statements_[i]->As<StatementBuilder>()) {
       statements_[i] = builder->Build(mod);
-      delete builder;
     }
   }
 
@@ -820,12 +805,10 @@ const ast::StatementList FunctionEmitter::ast_body() {
 
 ast::Statement* FunctionEmitter::AddStatement(ast::Statement* statement) {
   assert(!statements_stack_.empty());
-  auto* result = statement;
-  if (result != nullptr) {
-    auto& block = statements_stack_.back();
-    block.Add(statement);
+  if (statement != nullptr) {
+    statements_stack_.back().Add(statement);
   }
-  return result;
+  return statement;
 }
 
 ast::Statement* FunctionEmitter::LastStatement() {

src/reader/spirv/function.h

@@ -834,12 +834,15 @@ class FunctionEmitter {
   /// @returns a pointer to the statement.
   ast::Statement* AddStatement(ast::Statement* statement);
 
+  /// AddStatementBuilder() constructs and adds the StatementBuilder of type
+  /// `T` to the top of the statement stack.
+  /// @param args the arguments forwarded to the T constructor
+  /// @return the built StatementBuilder
   template <typename T, typename... ARGS>
   T* AddStatementBuilder(ARGS&&... args) {
-    // The builder is temporary and is not owned by the module.
-    auto builder = new T(std::forward<ARGS>(args)...);
-    AddStatement(builder);
-    return builder;
+    assert(!statements_stack_.empty());
+    return statements_stack_.back().AddStatementBuilder<T>(
+        std::forward<ARGS>(args)...);
   }
 
   /// Returns the source record for the given instruction.
@@ -876,6 +879,20 @@ class FunctionEmitter {
     /// Add() must not be called after calling Finalize().
     void Add(ast::Statement* statement);
 
+    /// AddStatementBuilder() constructs and adds the StatementBuilder of type
+    /// `T` to the block.
+    /// Add() must not be called after calling Finalize().
+    /// @param args the arguments forwarded to the T constructor
+    /// @return the built StatementBuilder
+    template <typename T, typename... ARGS>
+    T* AddStatementBuilder(ARGS&&... args) {
+      auto builder = std::make_unique<T>(std::forward<ARGS>(args)...);
+      auto* ptr = builder.get();
+      Add(ptr);
+      builders_.emplace_back(std::move(builder));
+      return ptr;
+    }
+
     /// @param construct the construct which this construct constributes to
     void SetConstruct(const Construct* construct) { construct_ = construct; }
 
@@ -883,7 +900,7 @@ class FunctionEmitter {
     const Construct* Construct() const { return construct_; }
 
     /// @return the ID of the block at which the completion action should be
-    /// triggerd and this statement block discarded. This is often the `end_id`
+    /// triggered and this statement block discarded. This is often the `end_id`
     /// of `construct` itself.
     uint32_t EndId() const { return end_id_; }
 
@@ -901,7 +918,7 @@ class FunctionEmitter {
    private:
     /// The construct to which this construct constributes.
     const spirv::Construct* construct_;
-    /// The ID of the block at which the completion action should be triggerd
+    /// The ID of the block at which the completion action should be triggered
     /// and this statement block discarded. This is often the `end_id` of
     /// `construct` itself.
     uint32_t const end_id_;
@@ -914,6 +931,9 @@ class FunctionEmitter {
     ast::StatementList statements_;
     /// The list of switch cases being built, if this construct is a switch.
     ast::CaseStatementList* cases_ = nullptr;
+
+    /// Owned statement builders
+    std::vector<std::unique_ptr<StatementBuilder>> builders_;
     /// True if Finalize() has been called.
     bool finalized_ = false;
   };

src/reader/wgsl/parser_impl.cc

@@ -1439,14 +1439,14 @@ Expect<ast::Expression*> ParserImpl::expect_paren_rhs_stmt() {
 //   : statement*
 Expect<ast::BlockStatement*> ParserImpl::expect_statements() {
   bool errored = false;
-  auto* ret = create<ast::BlockStatement>(Source{});
+  ast::StatementList stmts;
 
   while (synchronized_) {
     auto stmt = statement();
     if (stmt.errored) {
       errored = true;
     } else if (stmt.matched) {
-      ret->append(stmt.value);
+      stmts.emplace_back(stmt.value);
     } else {
       break;
     }
@@ -1455,7 +1455,7 @@ Expect<ast::BlockStatement*> ParserImpl::expect_statements() {
   if (errored)
     return Failure::kErrored;
 
-  return ret;
+  return create<ast::BlockStatement>(Source{}, stmts);
 }
 
 // statement
@@ -1828,14 +1828,14 @@ Expect<ast::CaseSelectorList> ParserImpl::expect_case_selectors() {
 //   | statement case_body
 //   | FALLTHROUGH SEMICOLON
 Maybe<ast::BlockStatement*> ParserImpl::case_body() {
-  auto* ret = create<ast::BlockStatement>(Source{});
+  ast::StatementList stmts;
   for (;;) {
     Source source;
     if (match(Token::Type::kFallthrough, &source)) {
       if (!expect("fallthrough statement", Token::Type::kSemicolon))
         return Failure::kErrored;
 
-      ret->append(create<ast::FallthroughStatement>(source));
+      stmts.emplace_back(create<ast::FallthroughStatement>(source));
       break;
     }
 
@@ -1845,10 +1845,10 @@ Maybe<ast::BlockStatement*> ParserImpl::case_body() {
     if (!stmt.matched)
       break;
 
-    ret->append(stmt.value);
+    stmts.emplace_back(stmt.value);
   }
 
-  return ret;
+  return create<ast::BlockStatement>(Source{}, stmts);
 }
 
 // loop_stmt
@@ -1972,8 +1972,10 @@ Maybe<ast::Statement*> ParserImpl::for_stmt() {
         header->condition->source(), ast::UnaryOp::kNot, header->condition);
     // { break; }
     auto* break_stmt = create<ast::BreakStatement>(not_condition->source());
-    auto* break_body = create<ast::BlockStatement>(not_condition->source());
-    break_body->append(break_stmt);
+    auto* break_body =
+        create<ast::BlockStatement>(not_condition->source(), ast::StatementList{
+                                                                 break_stmt,
+                                                             });
     // if (!condition) { break; }
     auto* break_if_not_condition =
         create<ast::IfStatement>(not_condition->source(), not_condition,
@@ -1983,17 +1985,19 @@ Maybe<ast::Statement*> ParserImpl::for_stmt() {
 
   ast::BlockStatement* continuing_body = nullptr;
   if (header->continuing != nullptr) {
-    continuing_body = create<ast::BlockStatement>(header->continuing->source());
-    continuing_body->append(header->continuing);
+    continuing_body = create<ast::BlockStatement>(header->continuing->source(),
+                                                  ast::StatementList{
+                                                      header->continuing,
+                                                  });
   }
 
   auto* loop = create<ast::LoopStatement>(source, body.value, continuing_body);
 
   if (header->initializer != nullptr) {
-    auto* result = create<ast::BlockStatement>(source);
-    result->append(header->initializer);
-    result->append(loop);
-    return result;
+    return create<ast::BlockStatement>(source, ast::StatementList{
+                                                   header->initializer,
+                                                   loop,
+                                               });
   }
 
   return loop;
@@ -2059,7 +2063,7 @@ Maybe<ast::ContinueStatement*> ParserImpl::continue_stmt() {
 //   : CONTINUING body_stmt
 Maybe<ast::BlockStatement*> ParserImpl::continuing_stmt() {
   if (!match(Token::Type::kContinuing))
-    return create<ast::BlockStatement>(Source{});
+    return create<ast::BlockStatement>(Source{}, ast::StatementList{});
 
   return expect_body_stmt();
 }