From 604bc72dd91d57841ba21cc4fa043d20d69d367c Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Sat, 12 Dec 2020 01:24:53 +0000 Subject: [PATCH] ast: Remove Node::set_source() All nodes that don't have a Source constructor will need to have one added. We can then find all missing Source mappings with a search for `Source{}` Bug: tint:396 Bug: tint:390 Change-Id: I06f9689d4da0f3fd1bd757c7358dcc65f15dc752 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/35018 Commit-Queue: Ben Clayton Reviewed-by: dan sinclair --- src/ast/node.h | 3 --- src/ast/test_helper.h | 1 + src/reader/spirv/function.cc | 49 +++++++++--------------------------- src/reader/spirv/function.h | 18 +++---------- src/type_determiner_test.cc | 11 ++++---- 5 files changed, 21 insertions(+), 61 deletions(-) diff --git a/src/ast/node.h b/src/ast/node.h index 5800086254..f72c41aebe 100644 --- a/src/ast/node.h +++ b/src/ast/node.h @@ -47,9 +47,6 @@ class Node : public Castable { /// @returns the node source data const Source& source() const { return source_; } - /// Sets the source data - /// @param source the source data - void set_source(const Source& source) { source_ = source; } /// @returns true if the node is valid virtual bool IsValid() const = 0; diff --git a/src/ast/test_helper.h b/src/ast/test_helper.h index 126171f5c0..5bb3402352 100644 --- a/src/ast/test_helper.h +++ b/src/ast/test_helper.h @@ -16,6 +16,7 @@ #define SRC_AST_TEST_HELPER_H_ #include +#include #include #include "gtest/gtest.h" diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 08a1c6d994..e5d22bdca2 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -608,12 +608,6 @@ class StructuredTraverser { std::unordered_set visited_; }; -/// @param src a source record -/// @returns true if `src` is a non-default Source -bool HasSource(const Source& src) { - return src.range.begin.line > 0 || src.range.begin.column != 0; -} - } // namespace BlockInfo::BlockInfo(const spvtools::opt::BasicBlock& bb) @@ -722,14 +716,6 @@ ast::Statement* FunctionEmitter::AddStatement(ast::Statement* statement) { return result; } -ast::Statement* FunctionEmitter::AddStatementForInstruction( - ast::Statement* statement, - const spvtools::opt::Instruction& inst) { - auto* node = AddStatement(statement); - ApplySourceForInstruction(node, inst); - return node; -} - ast::Statement* FunctionEmitter::LastStatement() { assert(!statements_stack_.empty()); auto* statement_list = statements_stack_.back().statements_; @@ -1880,7 +1866,7 @@ bool FunctionEmitter::EmitFunctionVariables() { inst.result_id(), ast::StorageClass::kFunction, var_store_type, false, constructor, ast::VariableDecorationList{}); auto* var_decl_stmt = create(var); - AddStatementForInstruction(var_decl_stmt, inst); + AddStatement(var_decl_stmt); // Save this as an already-named value. identifier_values_.insert(inst.result_id()); } @@ -2762,8 +2748,7 @@ bool FunctionEmitter::EmitConstDefinition( if (!ast_const) { return false; } - AddStatementForInstruction(create(ast_const), - inst); + AddStatement(create(ast_const)); // Save this as an already-named value. identifier_values_.insert(inst.result_id()); return success(); @@ -2777,11 +2762,10 @@ bool FunctionEmitter::EmitConstDefOrWriteToHoistedVar( if (def_info && def_info->requires_hoisted_def) { auto name = namer_.Name(result_id); // Emit an assignment of the expression to the hoisted variable. - AddStatementForInstruction(create( - create( - ast_module_.RegisterSymbol(name), name), - ast_expr.expr), - inst); + AddStatement(create( + create(ast_module_.RegisterSymbol(name), + namer_.Name(result_id)), + ast_expr.expr)); return true; } return EmitConstDefinition(inst, ast_expr); @@ -2848,8 +2832,7 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) { // TODO(dneto): Order of evaluation? auto lhs = MakeExpression(ptr_id); auto rhs = MakeExpression(value_id); - AddStatementForInstruction( - create(lhs.expr, rhs.expr), inst); + AddStatement(create(lhs.expr, rhs.expr)); return success(); } case SpvOpLoad: { @@ -3751,8 +3734,7 @@ bool FunctionEmitter::EmitFunctionCall(const spvtools::opt::Instruction& inst) { } if (result_type->Is()) { - return nullptr != AddStatementForInstruction( - create(call_expr), inst); + return nullptr != AddStatement(create(call_expr)); } return EmitConstDefOrWriteToHoistedVar(inst, {result_type, call_expr}); @@ -3816,16 +3798,9 @@ TypedExpression FunctionEmitter::MakeSimpleSelect( return {}; } -void FunctionEmitter::ApplySourceForInstruction( - ast::Node* node, - const spvtools::opt::Instruction& inst) { - if (!node) { - return; - } - const Source& existing = node->source(); - if (!HasSource(existing)) { - node->set_source(parser_impl_.GetSourceForInst(&inst)); - } +Source FunctionEmitter::GetSourceForInst( + const spvtools::opt::Instruction& inst) const { + return parser_impl_.GetSourceForInst(&inst); } bool FunctionEmitter::EmitImageAccess(const spvtools::opt::Instruction& inst) { @@ -4028,7 +4003,7 @@ bool FunctionEmitter::EmitImageAccess(const spvtools::opt::Instruction& inst) { } else { // It's an image write. No value is returned, so make a statement out // of the call. - AddStatementForInstruction(create(call_expr), inst); + AddStatement(create(call_expr)); } return success(); } diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index b0aee52c13..8f8eeba5b2 100644 --- a/src/reader/spirv/function.h +++ b/src/reader/spirv/function.h @@ -803,22 +803,10 @@ class FunctionEmitter { /// @returns a pointer to the statement. ast::Statement* AddStatement(ast::Statement* statement); - /// Appends a new statement to the top of the statement stack, and attaches - /// source location information from the given instruction. Does nothing if - /// the statement is null. - /// @param statement the new statement - /// @returns a pointer to the statement. - ast::Statement* AddStatementForInstruction( - ast::Statement* statement, - const spvtools::opt::Instruction& inst); - - /// Sets the source information for the given instruction to the given - /// node, if the node doesn't already have a source record. Does nothing - /// if `node` is null. - /// @param node the AST node + /// Returns the source record for the given instruction. /// @param inst the SPIR-V instruction - void ApplySourceForInstruction(ast::Node* node, - const spvtools::opt::Instruction& inst); + /// @return the Source record, or a default one + Source GetSourceForInst(const spvtools::opt::Instruction& inst) const; /// @returns the last statetment in the top of the statement stack. ast::Statement* LastStatement(); diff --git a/src/type_determiner_test.cc b/src/type_determiner_test.cc index 2372ef561c..1749a19368 100644 --- a/src/type_determiner_test.cc +++ b/src/type_determiner_test.cc @@ -73,6 +73,7 @@ namespace { class FakeStmt : public ast::Statement { public: + explicit FakeStmt(Source source) : ast::Statement(source) {} FakeStmt* Clone(ast::CloneContext*) const override { return nullptr; } bool IsValid() const override { return true; } void to_str(std::ostream& out, size_t) const override { out << "Fake"; } @@ -80,6 +81,7 @@ class FakeStmt : public ast::Statement { class FakeExpr : public ast::Expression { public: + explicit FakeExpr(Source source) : ast::Expression(source) {} FakeExpr* Clone(ast::CloneContext*) const override { return nullptr; } bool IsValid() const override { return true; } void to_str(std::ostream&, size_t) const override {} @@ -106,8 +108,7 @@ class TypeDeterminerTestWithParam : public TypeDeterminerHelper, public testing::TestWithParam {}; TEST_F(TypeDeterminerTest, Error_WithEmptySource) { - FakeStmt s; - s.set_source(Source{}); + FakeStmt s(Source{}); EXPECT_FALSE(td()->DetermineResultType(&s)); EXPECT_EQ(td()->error(), @@ -115,8 +116,7 @@ TEST_F(TypeDeterminerTest, Error_WithEmptySource) { } TEST_F(TypeDeterminerTest, Stmt_Error_Unknown) { - FakeStmt s; - s.set_source(Source{Source::Location{2, 30}}); + FakeStmt s(Source{Source::Location{2, 30}}); EXPECT_FALSE(td()->DetermineResultType(&s)); EXPECT_EQ(td()->error(), @@ -432,8 +432,7 @@ TEST_F(TypeDeterminerTest, Stmt_VariableDecl_ModuleScope) { } TEST_F(TypeDeterminerTest, Expr_Error_Unknown) { - FakeExpr e; - e.set_source(Source{Source::Location{2, 30}}); + FakeExpr e(Source{Source::Location{2, 30}}); EXPECT_FALSE(td()->DetermineResultType(&e)); EXPECT_EQ(td()->error(), "2:30: unknown expression for type determination");