From dc7839e8937f585d26432455c66dc2dc9a6433f1 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 10 Jun 2025 16:21:17 -0700 Subject: [PATCH] Add a new facility GrowingRange for a range that might grow during iteration. (#5641) Use it to replace most existing modernize-loop-convert lints with range-based for loops. As requested in review of #5475. --------- Co-authored-by: Jon Ross-Perkins --- common/BUILD | 16 ++++++++ common/growing_range.h | 68 ++++++++++++++++++++++++++++++++++ common/growing_range_test.cpp | 49 ++++++++++++++++++++++++ toolchain/check/BUILD | 2 + toolchain/check/check_unit.cpp | 13 ++----- toolchain/check/import_ref.cpp | 6 +-- toolchain/lower/BUILD | 1 + toolchain/lower/context.cpp | 9 ++--- 8 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 common/growing_range.h create mode 100644 common/growing_range_test.cpp diff --git a/common/BUILD b/common/BUILD index ef4fc258e334..032b1a17a6ca 100644 --- a/common/BUILD +++ b/common/BUILD @@ -236,6 +236,22 @@ cc_test( ], ) +cc_library( + name = "growing_range", + hdrs = ["growing_range.h"], +) + +cc_test( + name = "growing_range_test", + size = "small", + srcs = ["growing_range_test.cpp"], + deps = [ + ":growing_range", + "//testing/base:gtest_main", + "@googletest//:gtest", + ], +) + cc_library( name = "hashing", srcs = ["hashing.cpp"], diff --git a/common/growing_range.h b/common/growing_range.h new file mode 100644 index 000000000000..4ea660930b31 --- /dev/null +++ b/common/growing_range.h @@ -0,0 +1,68 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#ifndef CARBON_COMMON_GROWING_RANGE_H_ +#define CARBON_COMMON_GROWING_RANGE_H_ + +#include + +namespace Carbon { + +// A range adaptor for a random-access container such as `std::vector` or +// `llvm::SmallVector` that might have elements appended during the iteration. +// This adaptor avoids invalidation issues by tracking an index instead of an +// iterator, and by returning by value from `operator*` instead of by reference. +// +// This class is intended only for use as the range in a range-based for loop, +// and as such does not provide a complete range or iterator interface. Instead, +// it provides only the interface required by the range-based for loop. +template + requires std::ranges::sized_range && + std::ranges::random_access_range +class GrowingRange { + public: + // An end sentinel for the range. + class End {}; + + // An iterator into a potentially-growing range. Tracks the container and the + // current index, and indexes the container on each dereference. + class Iterator { + public: + // Dereferences the iterator. These intentionally don't return by reference, + // to avoid handing out a reference that would be invalidated when the + // container grows during the traversal. + auto operator*() -> auto { return (*container_)[index_]; } + auto operator*() const -> auto { return (*container_)[index_]; } + + friend auto operator!=(Iterator it, End /*end*/) -> bool { + return it.index_ != it.container_->size(); + } + + auto operator++() -> void { ++index_; } + + private: + friend class GrowingRange; + + explicit Iterator(const ContainerT* container) + : container_(container), index_(0) {} + + const ContainerT* container_; + size_t index_; + }; + + explicit GrowingRange(const ContainerT& container) : container_(&container) {} + + auto begin() const -> Iterator { return Iterator(container_); } + auto end() const -> End { return {}; } + + private: + const ContainerT* container_; +}; + +template +GrowingRange(const ContainerT&) -> GrowingRange; + +} // namespace Carbon + +#endif // CARBON_COMMON_GROWING_RANGE_H_ diff --git a/common/growing_range_test.cpp b/common/growing_range_test.cpp new file mode 100644 index 000000000000..a663baa596bc --- /dev/null +++ b/common/growing_range_test.cpp @@ -0,0 +1,49 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "common/growing_range.h" + +#include + +#include + +namespace Carbon { +namespace { + +TEST(GrowingRangeTest, TestUnchanged) { + std::vector v = {1, 2, 3, 4, 5}; + int k = 0; + for (int n : GrowingRange(v)) { + EXPECT_EQ(n, ++k); + } +} + +TEST(GrowingRangeTest, TestGrowWithRealloc) { + std::vector expected = {3, 2, 2, 2, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0}; + + std::vector v; + v.reserve(1); + v.push_back(3); + EXPECT_LT(v.capacity(), expected.size()); + + int i = 0; + for (int n : GrowingRange(v)) { + // Append n copies of n - 1. + v.insert(v.end(), n, n - 1); + EXPECT_EQ(n, expected[i++]); + } +} + +TEST(GrowingRangeTest, TestNoReference) { + std::vector v; + // Use `decltype(auto)` to capture the type of the element including whether + // it's a reference. + for (decltype(auto) elem : GrowingRange(v)) { + // The type of `elem` should be `int`, not `int&`. + static_assert(std::same_as); + } +} + +} // namespace +} // namespace Carbon diff --git a/toolchain/check/BUILD b/toolchain/check/BUILD index 221c3014b56d..c9cca5102304 100644 --- a/toolchain/check/BUILD +++ b/toolchain/check/BUILD @@ -113,6 +113,7 @@ cc_library( "//common:concepts", "//common:emplace_by_calling", "//common:find", + "//common:growing_range", "//common:map", "//common:ostream", "//common:raw_string_ostream", @@ -181,6 +182,7 @@ cc_library( "//common:check", "//common:error", "//common:find", + "//common:growing_range", "//common:map", "//common:ostream", "//common:vlog", diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index c34651fa6f1d..ed629ac1fe03 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -9,6 +9,7 @@ #include #include +#include "common/growing_range.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/VirtualFileSystem.h" @@ -454,11 +455,7 @@ auto CheckUnit::CheckRequiredDeclarations() -> void { auto CheckUnit::CheckRequiredDefinitions() -> void { CARBON_DIAGNOSTIC(MissingDefinitionInImpl, Error, "no definition found for declaration in impl file"); - - // Note that more required definitions can be added during this loop. - // NOLINTNEXTLINE(modernize-loop-convert) - for (size_t i = 0; i != context_.definitions_required_by_decl().size(); ++i) { - SemIR::InstId decl_inst_id = context_.definitions_required_by_decl()[i]; + for (SemIR::InstId decl_inst_id : context_.definitions_required_by_decl()) { SemIR::Inst decl_inst = context_.insts().Get(decl_inst_id); CARBON_KIND_SWITCH(context_.insts().Get(decl_inst_id)) { case CARBON_KIND(SemIR::ClassDecl class_decl): { @@ -497,12 +494,10 @@ auto CheckUnit::CheckRequiredDefinitions() -> void { } } - // Note that more required definitions can be added during this loop. - // NOLINTNEXTLINE(modernize-loop-convert) - for (size_t i = 0; i != context_.definitions_required_by_use().size(); ++i) { + for (auto [loc, specific_id] : + GrowingRange(context_.definitions_required_by_use())) { // This is using the location for the use. We could track the // list of enclosing locations if this was used from a generic. - auto [loc, specific_id] = context_.definitions_required_by_use()[i]; if (!ResolveSpecificDefinition(context_, loc, specific_id)) { CARBON_DIAGNOSTIC(MissingGenericFunctionDefinition, Error, "use of undefined generic function"); diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index 88bf98bd26af..01518854485a 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -10,6 +10,7 @@ #include #include "common/check.h" +#include "common/growing_range.h" #include "toolchain/base/kind_switch.h" #include "toolchain/check/context.h" #include "toolchain/check/eval.h" @@ -3278,9 +3279,8 @@ auto ImportRefResolver::PerformPendingWork() -> void { // state. // TODO: Import the generic eval block rather than calling // RebuildGenericEvalBlock to rebuild it so that order doesn't matter. - // NOLINTNEXTLINE(modernize-loop-convert) - for (size_t i = 0; i != pending_generics().size(); ++i) { - FinishPendingGeneric(*this, pending_generics()[i]); + for (auto generic_id : GrowingRange(pending_generics())) { + FinishPendingGeneric(*this, generic_id); } pending_generics().clear(); diff --git a/toolchain/lower/BUILD b/toolchain/lower/BUILD index a8988aedc58f..7040c27f1f3b 100644 --- a/toolchain/lower/BUILD +++ b/toolchain/lower/BUILD @@ -47,6 +47,7 @@ cc_library( ], deps = [ "//common:check", + "//common:growing_range", "//common:map", "//common:raw_string_ostream", "//common:vlog", diff --git a/toolchain/lower/context.cpp b/toolchain/lower/context.cpp index dba952eb7fc1..12e46569669b 100644 --- a/toolchain/lower/context.cpp +++ b/toolchain/lower/context.cpp @@ -5,6 +5,7 @@ #include "toolchain/lower/context.h" #include "common/check.h" +#include "common/growing_range.h" #include "common/vlog.h" #include "llvm/Transforms/Utils/ModuleUtils.h" #include "toolchain/lower/file_context.h" @@ -43,12 +44,8 @@ auto Context::GetFileContext(const SemIR::File* file, auto Context::LowerPendingDefinitions() -> void { // Lower function definitions for generics. - // This cannot be a range-based loop, as new definitions can be added - // while building other definitions. - // NOLINTNEXTLINE(modernize-loop-convert) - for (size_t i = 0; i != specific_function_definitions_.size(); ++i) { - auto [file_context, function_id, specific_id] = - specific_function_definitions_[i]; + for (auto [file_context, function_id, specific_id] : + GrowingRange(specific_function_definitions_)) { file_context->BuildFunctionDefinition(function_id, specific_id); } }