From 49334b05cf77d272b1eb479ad8afb1e111633365 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Tue, 6 Dec 2022 19:39:02 +0000 Subject: [PATCH] tint/utils: Remove non-const accessors on VectorRef Nothing uses these, and the mutability of these breaks const-correctness. Switch functions that used to return `const utils::Vector&` to returning `utils::VectorRef`. Removes the templated size from the public interface. Replace all `const utils::VectorRef&` with `utils::Vector`, there's no point in using yet another level of pointer indirection. Change-Id: Ib96e3171500606d9afffbb13f40023552a74fffc Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/113021 Kokoro: Kokoro Commit-Queue: Ben Clayton Reviewed-by: Dan Sinclair --- src/tint/inspector/inspector.cc | 2 +- src/tint/inspector/inspector.h | 2 +- src/tint/inspector/inspector_test.cc | 25 +++++++------- src/tint/ir/builder.cc | 2 +- src/tint/ir/builder.h | 2 +- src/tint/ir/switch.h | 2 +- src/tint/resolver/resolver.cc | 2 +- src/tint/resolver/validator.cc | 6 ++-- src/tint/resolver/validator.h | 6 ++-- src/tint/sem/function.h | 4 +-- src/tint/sem/module.h | 2 +- src/tint/sem/struct.h | 2 +- src/tint/sem/type.cc | 2 +- src/tint/transform/direct_variable_access.cc | 3 +- src/tint/utils/transform.h | 5 ++- src/tint/utils/unique_vector.h | 4 +-- src/tint/utils/unique_vector_test.cc | 6 ++-- src/tint/utils/vector.h | 35 ++++++-------------- src/tint/utils/vector_test.cc | 18 ---------- src/tint/writer/spirv/builder_type_test.cc | 4 +-- 20 files changed, 50 insertions(+), 84 deletions(-) diff --git a/src/tint/inspector/inspector.cc b/src/tint/inspector/inspector.cc index 25cde92d05..ea5b3ed7fc 100644 --- a/src/tint/inspector/inspector.cc +++ b/src/tint/inspector/inspector.cc @@ -532,7 +532,7 @@ std::vector Inspector::GetExternalTextureResourceBindings( ResourceBinding::ResourceType::kExternalTexture); } -utils::Vector Inspector::GetSamplerTextureUses( +utils::VectorRef Inspector::GetSamplerTextureUses( const std::string& entry_point) { auto* func = FindEntryPointByName(entry_point); if (!func) { diff --git a/src/tint/inspector/inspector.h b/src/tint/inspector/inspector.h index 49e4bdf20b..62a1c73c22 100644 --- a/src/tint/inspector/inspector.h +++ b/src/tint/inspector/inspector.h @@ -126,7 +126,7 @@ class Inspector { /// @param entry_point name of the entry point to get information about. /// @returns vector of all of the sampler/texture sampling pairs that are used /// by that entry point. - utils::Vector GetSamplerTextureUses(const std::string& entry_point); + utils::VectorRef GetSamplerTextureUses(const std::string& entry_point); /// @param entry_point name of the entry point to get information about. /// @param placeholder the sampler binding point to use for texture-only diff --git a/src/tint/inspector/inspector_test.cc b/src/tint/inspector/inspector_test.cc index 05f3c196b0..83302e2188 100644 --- a/src/tint/inspector/inspector_test.cc +++ b/src/tint/inspector/inspector_test.cc @@ -12,7 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "gtest/gtest.h" +#include "gmock/gmock.h" + #include "src/tint/ast/call_statement.h" #include "src/tint/ast/disable_validation_attribute.h" #include "src/tint/ast/id_attribute.h" @@ -33,16 +34,13 @@ using namespace tint::number_suffixes; // NOLINT namespace tint::inspector { namespace { -// All the tests that descend from InspectorBuilder are expected to define their -// test state via building up the AST through InspectorBuilder and then generate -// the program with ::Build. -// The returned Inspector from ::Build can then be used to test expecations. +// All the tests that descend from InspectorBuilder are expected to define their test state via +// building up the AST through InspectorBuilder and then generate the program with ::Build. The +// returned Inspector from ::Build can then be used to test expectations. // -// All the tests that descend from InspectorRunner are expected to define their -// test state via a WGSL shader, which will be parsed to generate a Program and -// Inspector in ::Initialize. -// The returned Inspector from ::Initialize can then be used to test -// expecations. +// All the tests that descend from InspectorRunner are expected to define their test state via a +// WGSL shader, which will be parsed to generate a Program and Inspector in ::Initialize. The +// returned Inspector from ::Initialize can then be used to test expectations. class InspectorGetEntryPointTest : public InspectorBuilder, public testing::Test {}; @@ -3202,7 +3200,7 @@ fn main(@location(0) fragUV: vec2, })"; Inspector& inspector = Initialize(shader); - auto result = inspector.GetSamplerTextureUses("foo"); + inspector.GetSamplerTextureUses("foo"); ASSERT_TRUE(inspector.has_error()) << inspector.error(); } @@ -3224,7 +3222,8 @@ fn main(@location(0) fragUV: vec2, auto result_1 = inspector.GetSamplerTextureUses("main"); ASSERT_FALSE(inspector.has_error()) << inspector.error(); - EXPECT_EQ(result_0, result_1); + EXPECT_EQ((utils::Vector(result_0)), + (utils::Vector(result_1))); } TEST_F(InspectorGetSamplerTextureUsesTest, BothIndirect) { @@ -3641,7 +3640,7 @@ fn main(@location(0) fragUV: vec2, })"; Inspector& inspector = Initialize(shader); - auto result = inspector.GetSamplerTextureUses("main"); + inspector.GetSamplerTextureUses("main"); } } // namespace diff --git a/src/tint/ir/builder.cc b/src/tint/ir/builder.cc index 0f0200d617..35e4f700be 100644 --- a/src/tint/ir/builder.cc +++ b/src/tint/ir/builder.cc @@ -77,7 +77,7 @@ Switch* Builder::CreateSwitch(const ast::SwitchStatement* stmt) { return ir_switch; } -Block* Builder::CreateCase(Switch* s, const utils::VectorRef selectors) { +Block* Builder::CreateCase(Switch* s, utils::VectorRef selectors) { s->cases.Push(Switch::Case{selectors, CreateBlock()}); Block* b = s->cases.Back().start_target; diff --git a/src/tint/ir/builder.h b/src/tint/ir/builder.h index 3f2e011059..ceabfa6b31 100644 --- a/src/tint/ir/builder.h +++ b/src/tint/ir/builder.h @@ -78,7 +78,7 @@ class Builder { /// @param s the switch to create the case into /// @param selectors the case selectors for the case statement /// @returns the start block for the case flow node - Block* CreateCase(Switch* s, const utils::VectorRef selectors); + Block* CreateCase(Switch* s, utils::VectorRef selectors); /// Branches the given block to the given flow node. /// @param from the block to branch from diff --git a/src/tint/ir/switch.h b/src/tint/ir/switch.h index 3ea9c3f736..7fff0a0fe6 100644 --- a/src/tint/ir/switch.h +++ b/src/tint/ir/switch.h @@ -33,7 +33,7 @@ class Switch : public Castable { /// A case label in the struct struct Case { /// The case selector for this node - const utils::VectorRef selectors; + utils::Vector selectors; /// The start block for the case block. Block* start_target; }; diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index ca66ac2b3d..628bf45906 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -1745,7 +1745,7 @@ const sem::Type* Resolver::ConcreteType(const sem::Type* ty, return nullptr; }, [&](const sem::Struct* s) -> const sem::Type* { - if (auto& tys = s->ConcreteTypes(); !tys.IsEmpty()) { + if (auto tys = s->ConcreteTypes(); !tys.IsEmpty()) { return target_ty ? target_ty : tys[0]; } return nullptr; diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index e5c7c1a6bb..dcb9b4c307 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -1811,7 +1811,7 @@ bool Validator::Matrix(const sem::Matrix* ty, const Source& source) const { return true; } -bool Validator::PipelineStages(const utils::VectorRef entry_points) const { +bool Validator::PipelineStages(utils::VectorRef entry_points) const { auto backtrace = [&](const sem::Function* func, const sem::Function* entry_point) { if (func != entry_point) { TraverseCallChain(diagnostics_, entry_point, func, [&](const sem::Function* f) { @@ -1906,7 +1906,7 @@ bool Validator::PipelineStages(const utils::VectorRef entry_poin return true; } -bool Validator::PushConstants(const utils::VectorRef entry_points) const { +bool Validator::PushConstants(utils::VectorRef entry_points) const { for (auto* entry_point : entry_points) { // State checked and modified by check_push_constant so that it remembers previously seen // push_constant variables for an entry-point. @@ -2422,7 +2422,7 @@ bool Validator::CheckTypeAccessAddressSpace( const sem::Type* store_ty, ast::Access access, ast::AddressSpace address_space, - const utils::VectorRef attributes, + utils::VectorRef attributes, const Source& source) const { if (!AddressSpaceLayout(store_ty, address_space, source)) { return false; diff --git a/src/tint/resolver/validator.h b/src/tint/resolver/validator.h index 0f183bbcef..9f0664cfd2 100644 --- a/src/tint/resolver/validator.h +++ b/src/tint/resolver/validator.h @@ -135,12 +135,12 @@ class Validator { /// Validates pipeline stages /// @param entry_points the entry points to the module /// @returns true on success, false otherwise. - bool PipelineStages(const utils::VectorRef entry_points) const; + bool PipelineStages(utils::VectorRef entry_points) const; /// Validates push_constant variables /// @param entry_points the entry points to the module /// @returns true on success, false otherwise. - bool PushConstants(const utils::VectorRef entry_points) const; + bool PushConstants(utils::VectorRef entry_points) const; /// Validates aliases /// @param alias the alias to validate @@ -508,7 +508,7 @@ class Validator { bool CheckTypeAccessAddressSpace(const sem::Type* store_ty, ast::Access access, ast::AddressSpace address_space, - const utils::VectorRef attributes, + utils::VectorRef attributes, const Source& source) const; SymbolTable& symbols_; diag::List& diagnostics_; diff --git a/src/tint/sem/function.h b/src/tint/sem/function.h index 8f21f7444a..7b5eaff968 100644 --- a/src/tint/sem/function.h +++ b/src/tint/sem/function.h @@ -135,9 +135,7 @@ class Function final : public Castable { /// @returns the list of texture/sampler pairs that this function uses /// (directly or indirectly). - const utils::Vector& TextureSamplerPairs() const { - return texture_sampler_pairs_; - } + utils::VectorRef TextureSamplerPairs() const { return texture_sampler_pairs_; } /// @returns the list of direct calls to functions / builtins made by this /// function diff --git a/src/tint/sem/module.h b/src/tint/sem/module.h index b451c5bd52..216a2c4e54 100644 --- a/src/tint/sem/module.h +++ b/src/tint/sem/module.h @@ -39,7 +39,7 @@ class Module final : public Castable { ~Module() override; /// @returns the dependency-ordered global declarations for the module - const utils::Vector& DependencyOrderedDeclarations() const { + utils::VectorRef DependencyOrderedDeclarations() const { return dep_ordered_decls_; } diff --git a/src/tint/sem/struct.h b/src/tint/sem/struct.h index 831cf3e02c..f8788540a0 100644 --- a/src/tint/sem/struct.h +++ b/src/tint/sem/struct.h @@ -163,7 +163,7 @@ class Struct final : public Castable { /// @returns the conversion-rank ordered concrete versions of this abstract structure, or an /// empty vector if this structure is not abstract. /// @note only structures returned by builtins may be abstract (e.g. modf, frexp) - const utils::Vector& ConcreteTypes() const { return concrete_types_; } + utils::VectorRef ConcreteTypes() const { return concrete_types_; } private: ast::Struct const* const declaration_; diff --git a/src/tint/sem/type.cc b/src/tint/sem/type.cc index 3d25e7eb3c..51afc12a4a 100644 --- a/src/tint/sem/type.cc +++ b/src/tint/sem/type.cc @@ -239,7 +239,7 @@ uint32_t Type::ConversionRank(const Type* from, const Type* to) { return kNoConversion; }, [&](const Struct* from_str) { - auto& concrete_tys = from_str->ConcreteTypes(); + auto concrete_tys = from_str->ConcreteTypes(); for (size_t i = 0; i < concrete_tys.Length(); i++) { if (concrete_tys[i] == to) { return static_cast(i + 1); diff --git a/src/tint/transform/direct_variable_access.cc b/src/tint/transform/direct_variable_access.cc index d1c1339ef2..26c4aba845 100644 --- a/src/tint/transform/direct_variable_access.cc +++ b/src/tint/transform/direct_variable_access.cc @@ -228,7 +228,8 @@ struct DirectVariableAccess::State { // will have the pointer parameters replaced with an array of u32s, used to perform the // pointer indexing in the variant. // Function call pointer arguments are replaced with an array of these dynamic indices. - for (auto* decl : utils::Reverse(sem.Module()->DependencyOrderedDeclarations())) { + auto decls = sem.Module()->DependencyOrderedDeclarations(); + for (auto* decl : utils::Reverse(decls)) { if (auto* fn = sem.Get(decl)) { auto* fn_info = FnInfoFor(fn); ProcessFunction(fn, fn_info); diff --git a/src/tint/utils/transform.h b/src/tint/utils/transform.h index 2faca462c1..d96a4bbb2d 100644 --- a/src/tint/utils/transform.h +++ b/src/tint/utils/transform.h @@ -91,8 +91,7 @@ auto Transform(const Vector& in, TRANSFORMER&& transform) /// @tparam N the small-array size of the returned Vector /// @returns a new vector with each element of the source vector transformed by `transform`. template -auto Transform(const VectorRef& in, TRANSFORMER&& transform) - -> Vector { +auto Transform(VectorRef in, TRANSFORMER&& transform) -> Vector { const auto count = in.Length(); Vector result; result.Reserve(count); @@ -108,7 +107,7 @@ auto Transform(const VectorRef& in, TRANSFORMER&& transform) /// @tparam N the small-array size of the returned Vector /// @returns a new vector with each element of the source vector transformed by `transform`. template -auto Transform(const VectorRef& in, TRANSFORMER&& transform) +auto Transform(VectorRef in, TRANSFORMER&& transform) -> Vector { const auto count = in.Length(); Vector result; diff --git a/src/tint/utils/unique_vector.h b/src/tint/utils/unique_vector.h index bda090cea0..6cb8f88a07 100644 --- a/src/tint/utils/unique_vector.h +++ b/src/tint/utils/unique_vector.h @@ -87,8 +87,8 @@ struct UniqueVector { /// @returns an iterator to the end of the reversed vector auto rend() const { return vector.rend(); } - /// @returns a const reference to the internal vector - operator const Vector&() const { return vector; } + /// @returns a reference to the internal vector + operator VectorRef() const { return vector; } /// @returns the std::move()'d vector. /// @note The UniqueVector must not be used after calling this method diff --git a/src/tint/utils/unique_vector_test.cc b/src/tint/utils/unique_vector_test.cc index 9b015c2092..c198615313 100644 --- a/src/tint/utils/unique_vector_test.cc +++ b/src/tint/utils/unique_vector_test.cc @@ -94,11 +94,11 @@ TEST(UniqueVectorTest, AsVector) { unique_vec.Add(1); unique_vec.Add(2); - const utils::Vector& vec = unique_vec; - EXPECT_EQ(vec.Length(), 3u); + utils::VectorRef ref = unique_vec; + EXPECT_EQ(ref.Length(), 3u); EXPECT_EQ(unique_vec.IsEmpty(), false); int i = 0; - for (auto n : vec) { + for (auto n : ref) { EXPECT_EQ(n, i); i++; } diff --git a/src/tint/utils/vector.h b/src/tint/utils/vector.h index 2371136fc4..7e2c271633 100644 --- a/src/tint/utils/vector.h +++ b/src/tint/utils/vector.h @@ -680,11 +680,11 @@ template Vector(Ts...) -> Vector, sizeof...(Ts)>; /// VectorRef is a weak reference to a Vector, used to pass vectors as parameters, avoiding copies -/// between the caller and the callee. VectorRef can accept a Vector of any 'N' value, decoupling -/// the caller's vector internal size from the callee's vector size. A VectorRef tracks the usage of -/// moves either side of the call. If at the call site, a Vector argument is moved to a VectorRef -/// parameter, and within the callee, the VectorRef parameter is moved to a Vector, then the Vector -/// heap allocation will be moved. For example: +/// between the caller and the callee, or as an non-static sized accessor on a vector. VectorRef can +/// accept a Vector of any 'N' value, decoupling the caller's vector internal size from the callee's +/// vector size. A VectorRef tracks the usage of moves either side of the call. If at the call site, +/// a Vector argument is moved to a VectorRef parameter, and within the callee, the VectorRef +/// parameter is moved to a Vector, then the Vector heap allocation will be moved. For example: /// /// ``` /// void func_a() { @@ -698,6 +698,8 @@ Vector(Ts...) -> Vector, sizeof...(Ts)>; /// Vector vec(std::move(vec_ref)); /// } /// ``` +/// +/// Aside from this move pattern, a VectorRef provides an immutable reference to the Vector. template class VectorRef { /// The slice type used by this vector reference @@ -710,6 +712,9 @@ class VectorRef { } public: + /// Type of `T`. + using value_type = T; + /// Constructor - empty reference VectorRef() : slice_(EmptySlice()) {} @@ -804,39 +809,21 @@ class VectorRef { /// @returns true if the vector is empty. bool IsEmpty() const { return slice_.len == 0; } - /// @returns a reference to the first element in the vector - T& Front() { return slice_.Front(); } - /// @returns a reference to the first element in the vector const T& Front() const { return slice_.Front(); } - /// @returns a reference to the last element in the vector - T& Back() { return slice_.Back(); } - /// @returns a reference to the last element in the vector const T& Back() const { return slice_.Back(); } - /// @returns a pointer to the first element in the vector - T* begin() { return slice_.begin(); } - /// @returns a pointer to the first element in the vector const T* begin() const { return slice_.begin(); } - /// @returns a pointer to one past the last element in the vector - T* end() { return slice_.end(); } - /// @returns a pointer to one past the last element in the vector const T* end() const { return slice_.end(); } - /// @returns a reverse iterator starting with the last element in the vector - auto rbegin() { return slice_.rbegin(); } - /// @returns a reverse iterator starting with the last element in the vector auto rbegin() const { return slice_.rbegin(); } - /// @returns the end for a reverse iterator - auto rend() { return slice_.rend(); } - /// @returns the end for a reverse iterator auto rend() const { return slice_.rend(); } @@ -907,7 +894,7 @@ inline std::ostream& operator<<(std::ostream& o, const utils::Vector& vec) /// @param vec the vector reference /// @return the std::ostream so calls can be chained template -inline std::ostream& operator<<(std::ostream& o, const utils::VectorRef& vec) { +inline std::ostream& operator<<(std::ostream& o, utils::VectorRef vec) { o << "["; bool first = true; for (auto& el : vec) { diff --git a/src/tint/utils/vector_test.cc b/src/tint/utils/vector_test.cc index 74d15b774f..ed9a97a48c 100644 --- a/src/tint/utils/vector_test.cc +++ b/src/tint/utils/vector_test.cc @@ -2072,15 +2072,6 @@ TEST(TintVectorRefTest, IsEmpty) { } TEST(TintVectorRefTest, FrontBack) { - Vector vec{"front", "mid", "back"}; - VectorRef vec_ref(vec); - static_assert(!std::is_const_v>); - static_assert(!std::is_const_v>); - EXPECT_EQ(vec_ref.Front(), "front"); - EXPECT_EQ(vec_ref.Back(), "back"); -} - -TEST(TintVectorRefTest, ConstFrontBack) { Vector vec{"front", "mid", "back"}; const VectorRef vec_ref(vec); static_assert(std::is_const_v>); @@ -2090,15 +2081,6 @@ TEST(TintVectorRefTest, ConstFrontBack) { } TEST(TintVectorRefTest, BeginEnd) { - Vector vec{"front", "mid", "back"}; - VectorRef vec_ref(vec); - static_assert(!std::is_const_v>); - static_assert(!std::is_const_v>); - EXPECT_EQ(vec_ref.begin(), &vec[0]); - EXPECT_EQ(vec_ref.end(), &vec[0] + 3); -} - -TEST(TintVectorRefTest, ConstBeginEnd) { Vector vec{"front", "mid", "back"}; const VectorRef vec_ref(vec); static_assert(std::is_const_v>); diff --git a/src/tint/writer/spirv/builder_type_test.cc b/src/tint/writer/spirv/builder_type_test.cc index 4377c424df..4ab8aa0268 100644 --- a/src/tint/writer/spirv/builder_type_test.cc +++ b/src/tint/writer/spirv/builder_type_test.cc @@ -442,8 +442,8 @@ TEST_F(BuilderTest_Type, GenerateStruct_DecoratedMembers_ArraysOfMatrix) { auto* arr_arr_mat2x3_f32 = ty.array(ty.array(ty.mat2x3(), 1_u), 2_u); // Doubly nested array auto* arr_arr_mat2x3_f16 = - ty.array(ty.array(ty.mat2x3(), 1_u), 2_u); // Doubly nested array - auto* rtarr_mat4x4 = ty.array(ty.mat4x4()); // Runtime array + ty.array(ty.array(ty.mat2x3(), 1_u), 2_u); // Doubly nested array + auto* rtarr_mat4x4 = ty.array(ty.mat4x4()); // Runtime array auto* s = Structure( "S", utils::Vector{