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 <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/ast/node.h b/src/ast/node.h
index 5800086..f72c41a 100644
--- a/src/ast/node.h
+++ b/src/ast/node.h
@@ -47,9 +47,6 @@
/// @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 126171f..5bb3402 100644
--- a/src/ast/test_helper.h
+++ b/src/ast/test_helper.h
@@ -16,6 +16,7 @@
#define SRC_AST_TEST_HELPER_H_
#include <memory>
+#include <string>
#include <utility>
#include "gtest/gtest.h"
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 08a1c6d..e5d22bd 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -608,12 +608,6 @@
std::unordered_set<uint32_t> 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 @@
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 @@
inst.result_id(), ast::StorageClass::kFunction, var_store_type, false,
constructor, ast::VariableDecorationList{});
auto* var_decl_stmt = create<ast::VariableDeclStatement>(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 @@
if (!ast_const) {
return false;
}
- AddStatementForInstruction(create<ast::VariableDeclStatement>(ast_const),
- inst);
+ AddStatement(create<ast::VariableDeclStatement>(ast_const));
// Save this as an already-named value.
identifier_values_.insert(inst.result_id());
return success();
@@ -2777,11 +2762,10 @@
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<ast::AssignmentStatement>(
- create<ast::IdentifierExpression>(
- ast_module_.RegisterSymbol(name), name),
- ast_expr.expr),
- inst);
+ AddStatement(create<ast::AssignmentStatement>(
+ create<ast::IdentifierExpression>(ast_module_.RegisterSymbol(name),
+ namer_.Name(result_id)),
+ ast_expr.expr));
return true;
}
return EmitConstDefinition(inst, ast_expr);
@@ -2848,8 +2832,7 @@
// TODO(dneto): Order of evaluation?
auto lhs = MakeExpression(ptr_id);
auto rhs = MakeExpression(value_id);
- AddStatementForInstruction(
- create<ast::AssignmentStatement>(lhs.expr, rhs.expr), inst);
+ AddStatement(create<ast::AssignmentStatement>(lhs.expr, rhs.expr));
return success();
}
case SpvOpLoad: {
@@ -3751,8 +3734,7 @@
}
if (result_type->Is<ast::type::Void>()) {
- return nullptr != AddStatementForInstruction(
- create<ast::CallStatement>(call_expr), inst);
+ return nullptr != AddStatement(create<ast::CallStatement>(call_expr));
}
return EmitConstDefOrWriteToHoistedVar(inst, {result_type, call_expr});
@@ -3816,16 +3798,9 @@
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 @@
} else {
// It's an image write. No value is returned, so make a statement out
// of the call.
- AddStatementForInstruction(create<ast::CallStatement>(call_expr), inst);
+ AddStatement(create<ast::CallStatement>(call_expr));
}
return success();
}
diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h
index b0aee52..8f8eeba 100644
--- a/src/reader/spirv/function.h
+++ b/src/reader/spirv/function.h
@@ -803,22 +803,10 @@
/// @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 2372ef56..1749a19 100644
--- a/src/type_determiner_test.cc
+++ b/src/type_determiner_test.cc
@@ -73,6 +73,7 @@
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 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 @@
public testing::TestWithParam<T> {};
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, 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, 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");
