From 2f4096b0d71a5b042bd2ed0902e230108512fb6e Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Wed, 18 Nov 2020 20:58:20 +0000 Subject: [PATCH] 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 Auto-Submit: Ben Clayton Reviewed-by: dan sinclair --- src/ast/builder.cc | 9 +++++--- src/ast/builder.h | 21 ++++++++++++------- src/ast/module.h | 15 +++++++++++++ src/ast/test_helper.h | 10 ++++----- src/context.h | 15 ------------- src/inspector/inspector_test.cc | 4 ++-- src/reader/reader.h | 9 -------- src/reader/spirv/function.h | 5 ++--- src/reader/spirv/parser_impl.h | 9 ++++++++ src/reader/wgsl/parser_impl.h | 4 ++-- src/reader/wgsl/parser_impl_test_helper.h | 1 - .../bound_array_accessors_transform_test.cc | 4 ++-- src/transform/transformer.h | 4 ++-- .../vertex_pulling_transform_test.cc | 4 ++-- src/type_determiner_test.cc | 4 ++-- src/validator/validator_test_helper.h | 4 ++-- src/writer/hlsl/test_helper.h | 4 ++-- src/writer/msl/test_helper.h | 4 ++-- .../spirv/builder_function_decoration_test.cc | 12 +++++------ src/writer/spirv/builder_function_test.cc | 8 +++---- src/writer/spirv/builder_intrinsic_test.cc | 7 +++---- src/writer/spirv/test_helper.h | 6 ++---- src/writer/wgsl/test_helper.h | 4 ++-- 23 files changed, 85 insertions(+), 82 deletions(-) diff --git a/src/ast/builder.cc b/src/ast/builder.cc index de6b3ae15e..b4aacca589 100644 --- a/src/ast/builder.cc +++ b/src/ast/builder.cc @@ -25,7 +25,8 @@ TypesBuilder::TypesBuilder(TypeManager* tm) void_(tm->Get()), 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()) {} -BuilderWithContext::~BuilderWithContext() { +BuilderWithContextAndModule::BuilderWithContextAndModule() + : Builder(new Context(), new ast::Module()) {} +BuilderWithContextAndModule::~BuilderWithContextAndModule() { delete ctx; + delete mod; } } // namespace ast diff --git a/src/ast/builder.h b/src/ast/builder.h index 364bfd3334..dcee4a207c 100644 --- a/src/ast/builder.h +++ b/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)...)}; } - /// 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 T* create(ARGS&&... args) { - return ctx->create(std::forward(args)...); + return mod->create(std::forward(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`. -class BuilderWithContext : public Builder { +/// BuilderWithContextAndModule is a `Builder` that constructs and owns its +/// `Context` and `Module`. +class BuilderWithContextAndModule : public Builder { public: - BuilderWithContext(); - ~BuilderWithContext() override; + BuilderWithContextAndModule(); + ~BuilderWithContextAndModule() override; }; //! @cond Doxygen_Suppress diff --git a/src/ast/module.h b/src/ast/module.h index c0b1e7bb8a..6ec3c7860a 100644 --- a/src/ast/module.h +++ b/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 + T* create(ARGS&&... args) { + static_assert(std::is_base_of::value, + "T does not derive from ast::Node"); + auto uptr = std::make_unique(std::forward(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 constructed_types_; FunctionList functions_; + std::vector> ast_nodes_; }; } // namespace ast diff --git a/src/ast/test_helper.h b/src/ast/test_helper.h index c97284c973..18ad0cfc5d 100644 --- a/src/ast/test_helper.h +++ b/src/ast/test_helper.h @@ -19,7 +19,7 @@ #include #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 T* create(ARGS&&... args) { - return ctx.create(std::forward(args)...); + return mod.create(std::forward(args)...); } - /// The context - Context ctx; + /// The module + Module mod; }; using TestHelper = TestHelperBase; diff --git a/src/context.h b/src/context.h index edec3e7b03..300b1130f1 100644 --- a/src/context.h +++ b/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 - T* create(ARGS&&... args) { - static_assert(std::is_base_of::value, - "T does not derive from ast::Node"); - auto uptr = std::make_unique(std::forward(args)...); - auto ptr = uptr.get(); - ast_nodes_.emplace_back(std::move(uptr)); - return ptr; - } - private: TypeManager type_mgr_; std::unique_ptr namer_; - std::vector> ast_nodes_; }; } // namespace tint diff --git a/src/inspector/inspector_test.cc b/src/inspector/inspector_test.cc index 44871923ec..d1d39e6e00 100644 --- a/src/inspector/inspector_test.cc +++ b/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 T* create(ARGS&&... args) { - return ctx_.create(std::forward(args)...); + return mod_.create(std::forward(args)...); } private: diff --git a/src/reader/reader.h b/src/reader/reader.h index 249c8d2682..481752fb80 100644 --- a/src/reader/reader.h +++ b/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 - T* create(ARGS&&... args) const { - return ctx_.create(std::forward(args)...); - } - /// The Tint context object Context& ctx_; diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index e9c54ad5ab..3211dc92b0 100644 --- a/src/reader/spirv/function.h +++ b/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 T* create(ARGS&&... args) const { - auto& ctx = parser_impl_.context(); - return ctx.create(std::forward(args)...); + return ast_module_.create(std::forward(args)...); } ParserImpl& parser_impl_; diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index 48f8d20e17..f0a23ca738 100644 --- a/src/reader/spirv/parser_impl.h +++ b/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 + T* create(ARGS&&... args) { + return ast_module_.create(std::forward(args)...); + } + // The SPIR-V binary we're parsing std::vector spv_binary_; diff --git a/src/reader/wgsl/parser_impl.h b/src/reader/wgsl/parser_impl.h index daf4789c57..09c5f5da1d 100644 --- a/src/reader/wgsl/parser_impl.h +++ b/src/reader/wgsl/parser_impl.h @@ -734,13 +734,13 @@ class ParserImpl { Maybe for_header_initializer(); Maybe 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 T* create(ARGS&&... args) { - return ctx_.create(std::forward(args)...); + return module_.create(std::forward(args)...); } Context& ctx_; diff --git a/src/reader/wgsl/parser_impl_test_helper.h b/src/reader/wgsl/parser_impl_test_helper.h index d77a9094db..6a6f1f5c65 100644 --- a/src/reader/wgsl/parser_impl_test_helper.h +++ b/src/reader/wgsl/parser_impl_test_helper.h @@ -76,7 +76,6 @@ class ParserImplTestWithParam : public testing::TestWithParam { private: std::vector> files_; - std::unique_ptr impl; Context ctx_; }; diff --git a/src/transform/bound_array_accessors_transform_test.cc b/src/transform/bound_array_accessors_transform_test.cc index 78cd50c49c..6302423b1b 100644 --- a/src/transform/bound_array_accessors_transform_test.cc +++ b/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 T* create(ARGS&&... args) { - return ctx_.create(std::forward(args)...); + return mod_.create(std::forward(args)...); } private: diff --git a/src/transform/transformer.h b/src/transform/transformer.h index 7fbcb587e0..d1ec99054f 100644 --- a/src/transform/transformer.h +++ b/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 T* create(ARGS&&... args) { - return ctx_->create(std::forward(args)...); + return mod_->create(std::forward(args)...); } /// The context diff --git a/src/transform/vertex_pulling_transform_test.cc b/src/transform/vertex_pulling_transform_test.cc index 4da530c153..91b632b320 100644 --- a/src/transform/vertex_pulling_transform_test.cc +++ b/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 T* create(ARGS&&... args) { - return ctx_.create(std::forward(args)...); + return mod_->create(std::forward(args)...); } private: diff --git a/src/type_determiner_test.cc b/src/type_determiner_test.cc index 7a251518f8..c12c0d2fcb 100644 --- a/src/type_determiner_test.cc +++ b/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 T* create(ARGS&&... args) { - return ctx_.create(std::forward(args)...); + return mod_.create(std::forward(args)...); } private: diff --git a/src/validator/validator_test_helper.h b/src/validator/validator_test_helper.h index 92edc341dc..b4db450298 100644 --- a/src/validator/validator_test_helper.h +++ b/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 T* create(ARGS&&... args) { - return ctx_.create(std::forward(args)...); + return mod_.create(std::forward(args)...); } private: diff --git a/src/writer/hlsl/test_helper.h b/src/writer/hlsl/test_helper.h index b55f542e5b..aea3938da5 100644 --- a/src/writer/hlsl/test_helper.h +++ b/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 template T* create(ARGS&&... args) { - return ctx.create(std::forward(args)...); + return mod.create(std::forward(args)...); } /// The context diff --git a/src/writer/msl/test_helper.h b/src/writer/msl/test_helper.h index 149ba43b78..b336a47ff0 100644 --- a/src/writer/msl/test_helper.h +++ b/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 T* create(ARGS&&... args) { - return ctx.create(std::forward(args)...); + return mod.create(std::forward(args)...); } /// The context diff --git a/src/writer/spirv/builder_function_decoration_test.cc b/src/writer/spirv/builder_function_decoration_test.cc index 8b59b2ed43..161966cbfd 100644 --- a/src/writer/spirv/builder_function_decoration_test.cc +++ b/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_out); - mod.AddGlobalVariable(v_wg); + mod->AddGlobalVariable(v_in); + mod->AddGlobalVariable(v_out); + 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_out); - mod.AddGlobalVariable(v_wg); + mod->AddGlobalVariable(v_in); + mod->AddGlobalVariable(v_out); + mod->AddGlobalVariable(v_wg); ASSERT_TRUE(b.GenerateFunction(&func)) << b.error(); EXPECT_EQ(DumpInstructions(b.debug()), R"(OpName %1 "tint_6d795f696e" diff --git a/src/writer/spirv/builder_function_test.cc b/src/writer/spirv/builder_function_test.cc index 1a6de922ca..20cbf8a654 100644 --- a/src/writer/spirv/builder_function_test.cc +++ b/src/writer/spirv/builder_function_test.cc @@ -252,10 +252,10 @@ TEST_F(BuilderTest, Emit_Multiple_EntryPoint_With_Same_ModuleVar) { decos.push_back(create(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::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::PipelineStage::kCompute, Source{})); - mod.AddFunction(func); + mod->AddFunction(func); } ASSERT_TRUE(td.Determine()) << td.error(); diff --git a/src/writer/spirv/builder_intrinsic_test.cc b/src/writer/spirv/builder_intrinsic_test.cc index c877885dc8..3b056376c7 100644 --- a/src/writer/spirv/builder_intrinsic_test.cc +++ b/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}; - spirv::Builder b{ctx, &mod}; + TypeDeterminer td{ctx, mod}; + spirv::Builder b{ctx, mod}; }; template diff --git a/src/writer/spirv/test_helper.h b/src/writer/spirv/test_helper.h index 98637ba972..c8194b4edd 100644 --- a/src/writer/spirv/test_helper.h +++ b/src/writer/spirv/test_helper.h @@ -31,13 +31,11 @@ namespace spirv { /// Helper class for testing template -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 diff --git a/src/writer/wgsl/test_helper.h b/src/writer/wgsl/test_helper.h index 76e434f233..57b14ce502 100644 --- a/src/writer/wgsl/test_helper.h +++ b/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 T* create(ARGS&&... args) { - return ctx.create(std::forward(args)...); + return mod.create(std::forward(args)...); } /// The context