Move the ast node ownership from Context to Module

Fixes: tint:335
Change-Id: I128e229daa56d43e7227ecab72269be33b3ee012
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/33240
Commit-Queue: Ben Clayton <bclayton@google.com>
Auto-Submit: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

src/ast/builder.cc

@@ -25,7 +25,8 @@ TypesBuilder::TypesBuilder(TypeManager* tm)
       void_(tm->Get<ast::type::VoidType>()),
       tm_(tm) {}
 
-Builder::Builder(tint::Context* c) : ctx(c), ty(&c->type_mgr()) {}
+Builder::Builder(tint::Context* c, tint::ast::Module* m)
+    : ctx(c), mod(m), ty(&c->type_mgr()) {}
 Builder::~Builder() = default;
 
 ast::Variable* Builder::Var(const std::string& name,
@@ -36,9 +37,11 @@ ast::Variable* Builder::Var(const std::string& name,
   return var;
 }
 
-BuilderWithContext::BuilderWithContext() : Builder(new Context()) {}
+BuilderWithContextAndModule::BuilderWithContextAndModule()
-BuilderWithContext::~BuilderWithContext() {
+    : Builder(new Context(), new ast::Module()) {}
+BuilderWithContextAndModule::~BuilderWithContextAndModule() {
   delete ctx;
+  delete mod;
 }
 
 }  // namespace ast

src/ast/builder.h

@@ -24,6 +24,7 @@
 #include "src/ast/expression.h"
 #include "src/ast/float_literal.h"
 #include "src/ast/identifier_expression.h"
+#include "src/ast/module.h"
 #include "src/ast/scalar_constructor_expression.h"
 #include "src/ast/sint_literal.h"
 #include "src/ast/type/array_type.h"
@@ -185,7 +186,8 @@ class Builder {
 
   /// Constructor
   /// @param ctx the context to use in the builder
-  explicit Builder(tint::Context* ctx);
+  /// @param mod the module to use in the builder
+  explicit Builder(tint::Context* ctx, tint::ast::Module* mod);
   virtual ~Builder();
 
   /// @param expr the expression
@@ -441,17 +443,19 @@ class Builder {
                                ExprList(std::forward<ARGS>(args)...)};
   }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx->create<T>(std::forward<ARGS>(args)...);
+    return mod->create<T>(std::forward<ARGS>(args)...);
   }
 
-  /// The builder context
+  /// The builder module
   tint::Context* const ctx;
+  /// The builder module
+  tint::ast::Module* const mod;
   /// The builder types
   const TypesBuilder ty;
 
@@ -460,11 +464,12 @@ class Builder {
   virtual void OnVariableBuilt(ast::Variable*) {}
 };
 
-/// BuilderWithContext is a `Builder` that constructs and owns its `Context`.
+/// BuilderWithContextAndModule is a `Builder` that constructs and owns its
-class BuilderWithContext : public Builder {
+/// `Context` and `Module`.
+class BuilderWithContextAndModule : public Builder {
  public:
-  BuilderWithContext();
+  BuilderWithContextAndModule();
-  ~BuilderWithContext() override;
+  ~BuilderWithContextAndModule() override;
 };
 
 //! @cond Doxygen_Suppress

src/ast/module.h

@@ -77,6 +77,20 @@ class Module {
   /// @returns a string representation of the module
   std::string to_str() const;
 
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
+  /// destructed, the `ast::Node` will also be destructed.
+  /// @param args the arguments to pass to the type constructor
+  /// @returns the node pointer
+  template <typename T, typename... ARGS>
+  T* create(ARGS&&... args) {
+    static_assert(std::is_base_of<ast::Node, T>::value,
+                  "T does not derive from ast::Node");
+    auto uptr = std::make_unique<T>(std::forward<ARGS>(args)...);
+    auto ptr = uptr.get();
+    ast_nodes_.emplace_back(std::move(uptr));
+    return ptr;
+  }
+
  private:
   Module(const Module&) = delete;
 
@@ -84,6 +98,7 @@ class Module {
   // The constructed types are owned by the type manager
   std::vector<type::Type*> constructed_types_;
   FunctionList functions_;
+  std::vector<std::unique_ptr<ast::Node>> ast_nodes_;
 };
 
 }  // namespace ast

src/ast/test_helper.h

@@ -19,7 +19,7 @@
 #include <utility>
 
 #include "gtest/gtest.h"
-#include "src/context.h"
+#include "src/ast/module.h"
 
 namespace tint {
 namespace ast {
@@ -31,17 +31,17 @@ class TestHelperBase : public BASE {
   TestHelperBase() {}
   ~TestHelperBase() = default;
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx.create<T>(std::forward<ARGS>(args)...);
+    return mod.create<T>(std::forward<ARGS>(args)...);
   }
 
-  /// The context
+  /// The module
-  Context ctx;
+  Module mod;
 };
 using TestHelper = TestHelperBase<testing::Test>;
 

src/context.h

@@ -51,24 +51,9 @@ class Context {
   /// @returns the namer object
   Namer* namer() const { return namer_.get(); }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
-  /// destructed, the `ast::Node` will also be destructed.
-  /// @param args the arguments to pass to the type constructor
-  /// @returns the node pointer
-  template <typename T, typename... ARGS>
-  T* create(ARGS&&... args) {
-    static_assert(std::is_base_of<ast::Node, T>::value,
-                  "T does not derive from ast::Node");
-    auto uptr = std::make_unique<T>(std::forward<ARGS>(args)...);
-    auto ptr = uptr.get();
-    ast_nodes_.emplace_back(std::move(uptr));
-    return ptr;
-  }
-
  private:
   TypeManager type_mgr_;
   std::unique_ptr<Namer> namer_;
-  std::vector<std::unique_ptr<ast::Node>> ast_nodes_;
 };
 
 }  // namespace tint

src/inspector/inspector_test.cc

@@ -635,13 +635,13 @@ class InspectorHelper {
     return &comparison_sampler_type_;
   }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx_.create<T>(std::forward<ARGS>(args)...);
+    return mod_.create<T>(std::forward<ARGS>(args)...);
   }
 
  private:

src/reader/reader.h

@@ -59,15 +59,6 @@ class Reader {
   /// @param diags the list of diagnostic messages
   void set_diagnostics(const diag::List& diags) { diags_ = diags; }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
-  /// destructed, the `ast::Node` will also be destructed.
-  /// @param args the arguments to pass to the type constructor
-  /// @returns the node pointer
-  template <typename T, typename... ARGS>
-  T* create(ARGS&&... args) const {
-    return ctx_.create<T>(std::forward<ARGS>(args)...);
-  }
-
   /// The Tint context object
   Context& ctx_;
 

src/reader/spirv/function.h

@@ -797,14 +797,13 @@ class FunctionEmitter {
   /// @returns a boolean false expression.
   ast::Expression* MakeFalse() const;
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) const {
-    auto& ctx = parser_impl_.context();
+    return ast_module_.create<T>(std::forward<ARGS>(args)...);
-    return ctx.create<T>(std::forward<ARGS>(args)...);
   }
 
   ParserImpl& parser_impl_;

src/reader/spirv/parser_impl.h

@@ -450,6 +450,15 @@ class ParserImpl : Reader {
   bool ApplyArrayDecorations(const spvtools::opt::analysis::Type* spv_type,
                              ast::type::ArrayType* ast_type);
 
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
+  /// destructed, the `ast::Node` will also be destructed.
+  /// @param args the arguments to pass to the type constructor
+  /// @returns the node pointer
+  template <typename T, typename... ARGS>
+  T* create(ARGS&&... args) {
+    return ast_module_.create<T>(std::forward<ARGS>(args)...);
+  }
+
   // The SPIR-V binary we're parsing
   std::vector<uint32_t> spv_binary_;
 

src/reader/wgsl/parser_impl.h

@@ -734,13 +734,13 @@ class ParserImpl {
   Maybe<ast::Statement*> for_header_initializer();
   Maybe<ast::Statement*> for_header_continuing();
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx_.create<T>(std::forward<ARGS>(args)...);
+    return module_.create<T>(std::forward<ARGS>(args)...);
   }
 
   Context& ctx_;

src/reader/wgsl/parser_impl_test_helper.h

@@ -76,7 +76,6 @@ class ParserImplTestWithParam : public testing::TestWithParam<T> {
 
  private:
   std::vector<std::unique_ptr<Source::File>> files_;
-  std::unique_ptr<ParserImpl> impl;
   Context ctx_;
 };
 

src/transform/bound_array_accessors_transform_test.cc

@@ -76,13 +76,13 @@ class BoundArrayAccessorsTest : public testing::Test {
 
   Manager* manager() { return manager_.get(); }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx_.create<T>(std::forward<ARGS>(args)...);
+    return mod_.create<T>(std::forward<ARGS>(args)...);
   }
 
  private:

src/transform/transformer.h

@@ -44,13 +44,13 @@ class Transformer {
   const std::string& error() { return error_; }
 
  protected:
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx_->create<T>(std::forward<ARGS>(args)...);
+    return mod_->create<T>(std::forward<ARGS>(args)...);
   }
 
   /// The context

src/transform/vertex_pulling_transform_test.cc

@@ -86,13 +86,13 @@ class VertexPullingTransformHelper {
   Manager* manager() { return manager_.get(); }
   VertexPullingTransform* transform() { return transform_; }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx_.create<T>(std::forward<ARGS>(args)...);
+    return mod_->create<T>(std::forward<ARGS>(args)...);
   }
 
  private:

src/type_determiner_test.cc

@@ -88,13 +88,13 @@ class TypeDeterminerHelper {
   ast::Module* mod() { return &mod_; }
   Context* ctx() { return &ctx_; }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx_.create<T>(std::forward<ARGS>(args)...);
+    return mod_.create<T>(std::forward<ARGS>(args)...);
   }
 
  private:

src/validator/validator_test_helper.h

@@ -41,13 +41,13 @@ class ValidatorTestHelper {
   /// @return a pointer to the test module
   ast::Module* mod() { return &mod_; }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx_.create<T>(std::forward<ARGS>(args)...);
+    return mod_.create<T>(std::forward<ARGS>(args)...);
   }
 
  private:

src/writer/hlsl/test_helper.h

@@ -43,13 +43,13 @@ class TestHelperBase : public BODY {
   /// @returns the pre result string
   std::string pre_result() const { return pre.str(); }
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer  template <typename T, typename... ARGS>
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx.create<T>(std::forward<ARGS>(args)...);
+    return mod.create<T>(std::forward<ARGS>(args)...);
   }
 
   /// The context

src/writer/msl/test_helper.h

@@ -35,13 +35,13 @@ class TestHelperBase : public BASE {
   TestHelperBase() : td(&ctx, &mod), gen(&ctx, &mod) {}
   ~TestHelperBase() = default;
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx.create<T>(std::forward<ARGS>(args)...);
+    return mod.create<T>(std::forward<ARGS>(args)...);
   }
 
   /// The context

src/writer/spirv/builder_function_decoration_test.cc

@@ -105,9 +105,9 @@ TEST_F(BuilderTest, FunctionDecoration_Stage_WithUnusedInterfaceIds) {
   EXPECT_TRUE(b.GenerateGlobalVariable(v_out)) << b.error();
   EXPECT_TRUE(b.GenerateGlobalVariable(v_wg)) << b.error();
 
-  mod.AddGlobalVariable(v_in);
+  mod->AddGlobalVariable(v_in);
-  mod.AddGlobalVariable(v_out);
+  mod->AddGlobalVariable(v_out);
-  mod.AddGlobalVariable(v_wg);
+  mod->AddGlobalVariable(v_wg);
 
   ASSERT_TRUE(b.GenerateFunction(&func)) << b.error();
   EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %1 "tint_6d795f696e"
@@ -168,9 +168,9 @@ TEST_F(BuilderTest, FunctionDecoration_Stage_WithUsedInterfaceIds) {
   EXPECT_TRUE(b.GenerateGlobalVariable(v_out)) << b.error();
   EXPECT_TRUE(b.GenerateGlobalVariable(v_wg)) << b.error();
 
-  mod.AddGlobalVariable(v_in);
+  mod->AddGlobalVariable(v_in);
-  mod.AddGlobalVariable(v_out);
+  mod->AddGlobalVariable(v_out);
-  mod.AddGlobalVariable(v_wg);
+  mod->AddGlobalVariable(v_wg);
 
   ASSERT_TRUE(b.GenerateFunction(&func)) << b.error();
   EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %1 "tint_6d795f696e"

src/writer/spirv/builder_function_test.cc

@@ -252,10 +252,10 @@ TEST_F(BuilderTest, Emit_Multiple_EntryPoint_With_Same_ModuleVar) {
   decos.push_back(create<ast::SetDecoration>(0, Source{}));
   data_var->set_decorations(decos);
 
-  mod.AddConstructedType(&s);
+  mod->AddConstructedType(&s);
 
   td.RegisterVariableForTesting(data_var);
-  mod.AddGlobalVariable(data_var);
+  mod->AddGlobalVariable(data_var);
 
   {
     ast::VariableList params;
@@ -272,7 +272,7 @@ TEST_F(BuilderTest, Emit_Multiple_EntryPoint_With_Same_ModuleVar) {
     func->add_decoration(
         create<ast::StageDecoration>(ast::PipelineStage::kCompute, Source{}));
 
-    mod.AddFunction(func);
+    mod->AddFunction(func);
   }
 
   {
@@ -290,7 +290,7 @@ TEST_F(BuilderTest, Emit_Multiple_EntryPoint_With_Same_ModuleVar) {
     func->add_decoration(
         create<ast::StageDecoration>(ast::PipelineStage::kCompute, Source{}));
 
-    mod.AddFunction(func);
+    mod->AddFunction(func);
   }
 
   ASSERT_TRUE(td.Determine()) << td.error();

src/writer/spirv/builder_intrinsic_test.cc

@@ -51,16 +51,15 @@ namespace writer {
 namespace spirv {
 namespace {
 
-class IntrinsicBuilderTest : public ast::BuilderWithContext,
+class IntrinsicBuilderTest : public ast::BuilderWithContextAndModule,
                              public testing::Test {
  protected:
   void OnVariableBuilt(ast::Variable* var) override {
     td.RegisterVariableForTesting(var);
   }
 
-  ast::Module mod;
+  TypeDeterminer td{ctx, mod};
-  TypeDeterminer td{ctx, &mod};
+  spirv::Builder b{ctx, mod};
-  spirv::Builder b{ctx, &mod};
 };
 
 template <typename T>

src/writer/spirv/test_helper.h

@@ -31,13 +31,11 @@ namespace spirv {
 
 /// Helper class for testing
 template <typename BASE>
-class TestHelperBase : public ast::BuilderWithContext, public BASE {
+class TestHelperBase : public ast::BuilderWithContextAndModule, public BASE {
  public:
-  TestHelperBase() : td(ctx, &mod), b(ctx, &mod) {}
+  TestHelperBase() : td(ctx, mod), b(ctx, mod) {}
   ~TestHelperBase() override = default;
 
-  /// The module
-  ast::Module mod;
   /// The type determiner
   TypeDeterminer td;
   /// The generator

src/writer/wgsl/test_helper.h

@@ -36,13 +36,13 @@ class TestHelperBase : public BASE {
 
   ~TestHelperBase() = default;
 
-  /// Creates a new `ast::Node` owned by the Context. When the Context is
+  /// Creates a new `ast::Node` owned by the Module. When the Module is
   /// destructed, the `ast::Node` will also be destructed.
   /// @param args the arguments to pass to the type constructor
   /// @returns the node pointer
   template <typename T, typename... ARGS>
   T* create(ARGS&&... args) {
-    return ctx.create<T>(std::forward<ARGS>(args)...);
+    return mod.create<T>(std::forward<ARGS>(args)...);
   }
 
   /// The context