From d46e7f8c61194c8555a74e8f2cf32e8cb257a043 Mon Sep 17 00:00:00 2001 From: Javier Lopez-Gomez Date: Wed, 17 Sep 2025 19:05:01 +0200 Subject: [PATCH 1/2] [ADT] Simplify `llvm::concat_iterator` implementation Simplify `llvm::concat_iterator::increment()` and `llvm::concat_iterator::get()`. This makes it possible to also get rid of `handle_type`, which was causing some additional complications. --- llvm/include/llvm/ADT/STLExtras.h | 64 ++++++++++--------------------- 1 file changed, 20 insertions(+), 44 deletions(-) diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h index 02705fbc8f2c4..411520cd228e5 100644 --- a/llvm/include/llvm/ADT/STLExtras.h +++ b/llvm/include/llvm/ADT/STLExtras.h @@ -1000,10 +1000,6 @@ class concat_iterator using reference_type = typename std::conditional_t; - using handle_type = - typename std::conditional_t, - ValueT *>; - /// We store both the current and end iterators for each concatenated /// sequence in a tuple of pairs. /// @@ -1013,49 +1009,38 @@ class concat_iterator std::tuple Begins; std::tuple Ends; - /// Attempts to increment a specific iterator. - /// - /// Returns true if it was able to increment the iterator. Returns false if - /// the iterator is already at the end iterator. - template bool incrementHelper() { + /// Attempts to increment the `Index`-th iterator. If the iterator is already + /// at end, recurse over iterators in `Others...`. + template void incrementImpl() { auto &Begin = std::get(Begins); auto &End = std::get(Ends); - if (Begin == End) - return false; - + if (Begin == End) { + if constexpr (sizeof...(Others) != 0) + return incrementImpl(); + llvm_unreachable("Attempted to increment an end concat iterator!"); + } ++Begin; - return true; } /// Increments the first non-end iterator. /// /// It is an error to call this with all iterators at the end. template void increment(std::index_sequence) { - // Build a sequence of functions to increment each iterator if possible. - bool (concat_iterator::*IncrementHelperFns[])() = { - &concat_iterator::incrementHelper...}; - - // Loop over them, and stop as soon as we succeed at incrementing one. - for (auto &IncrementHelperFn : IncrementHelperFns) - if ((this->*IncrementHelperFn)()) - return; - - llvm_unreachable("Attempted to increment an end concat iterator!"); + incrementImpl(); } - /// Returns null if the specified iterator is at the end. Otherwise, - /// dereferences the iterator and returns the address of the resulting - /// reference. - template handle_type getHelper() const { + /// Dereferences the `Index`-th iterator and returns the resulting reference. + /// If `Index` is at end, recurse over iterators in `Others...`. + template handle_type getImpl() const { auto &Begin = std::get(Begins); auto &End = std::get(Ends); - if (Begin == End) - return {}; - - if constexpr (ReturnsByValue) - return *Begin; - else - return &*Begin; + if (Begin == End) { + if constexpr (sizeof...(Others) != 0) + return getImpl(); + llvm_unreachable( + "Attempted to get a pointer from an end concat iterator!"); + } + return *Begin; } /// Finds the first non-end iterator, dereferences, and returns the resulting @@ -1063,16 +1048,7 @@ class concat_iterator /// /// It is an error to call this with all iterators at the end. template reference_type get(std::index_sequence) const { - // Build a sequence of functions to get from iterator if possible. - handle_type (concat_iterator::*GetHelperFns[])() - const = {&concat_iterator::getHelper...}; - - // Loop over them, and return the first result we find. - for (auto &GetHelperFn : GetHelperFns) - if (auto P = (this->*GetHelperFn)()) - return *P; - - llvm_unreachable("Attempted to get a pointer from an end concat iterator!"); + return getImpl(); } public: From 17229ae96c3372105c2262ab63c98ab2e087642f Mon Sep 17 00:00:00 2001 From: Javier Lopez-Gomez Date: Wed, 17 Sep 2025 19:05:01 +0200 Subject: [PATCH 2/2] [ADT] Fix llvm::concat_iterator for `ValueT == common_base_class *` Fix llvm::concat_iterator for the case of `ValueT` being a pointer to a common base class to which the result of dereferencing any iterator in `ItersT` can be casted to. --- llvm/include/llvm/ADT/STLExtras.h | 21 ++++++++++-- llvm/unittests/ADT/STLExtrasTest.cpp | 48 ++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h index 411520cd228e5..4e7e42e9f4a5f 100644 --- a/llvm/include/llvm/ADT/STLExtras.h +++ b/llvm/include/llvm/ADT/STLExtras.h @@ -114,6 +114,13 @@ using is_one_of = std::disjunction...>; template using are_base_of = std::conjunction...>; +/// traits class for checking whether type `T` is same as all other types in +/// `Ts`. +template +using all_types_equal = std::conjunction...>; +template +constexpr bool all_types_equal_v = all_types_equal::value; + /// Determine if all types in Ts are distinct. /// /// Useful to statically assert when Ts is intended to describe a non-multi set @@ -996,9 +1003,17 @@ class concat_iterator static constexpr bool ReturnsByValue = !(std::is_reference_v())> && ...); - + static constexpr bool ReturnsConvertibleType = + !all_types_equal_v< + std::remove_cv_t, + remove_cvref_t())>...> && + (std::is_convertible_v()), ValueT> && ...); + + // Cannot return a reference type if a conversion takes place, provided that + // the result of dereferencing all `IterTs...` is convertible to `ValueT`. using reference_type = - typename std::conditional_t; + std::conditional_t; /// We store both the current and end iterators for each concatenated /// sequence in a tuple of pairs. @@ -1031,7 +1046,7 @@ class concat_iterator /// Dereferences the `Index`-th iterator and returns the resulting reference. /// If `Index` is at end, recurse over iterators in `Others...`. - template handle_type getImpl() const { + template reference_type getImpl() const { auto &Begin = std::get(Begins); auto &End = std::get(Ends); if (Begin == End) { diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp index d355a59973f9a..5020acda95b0b 100644 --- a/llvm/unittests/ADT/STLExtrasTest.cpp +++ b/llvm/unittests/ADT/STLExtrasTest.cpp @@ -398,6 +398,8 @@ struct some_struct { std::string swap_val; }; +struct derives_from_some_struct : some_struct {}; + std::vector::const_iterator begin(const some_struct &s) { return s.data.begin(); } @@ -500,6 +502,15 @@ TEST(STLExtrasTest, ToVector) { } } +TEST(STLExtrasTest, AllTypesEqual) { + static_assert(all_types_equal_v<>); + static_assert(all_types_equal_v); + static_assert(all_types_equal_v); + + static_assert(!all_types_equal_v); + static_assert(!all_types_equal_v); +} + TEST(STLExtrasTest, ConcatRange) { std::vector Expected = {1, 2, 3, 4, 5, 6, 7, 8}; std::vector Test; @@ -532,6 +543,43 @@ TEST(STLExtrasTest, ConcatRangeADL) { EXPECT_THAT(concat(S0, S1), ElementsAre(1, 2, 3, 4)); } +TEST(STLExtrasTest, ConcatRangePtrToSameClass) { + some_namespace::some_struct S0{}; + some_namespace::some_struct S1{}; + SmallVector V0{&S0}; + SmallVector V1{&S1, &S1}; + + // Dereferencing all iterators yields `some_namespace::some_struct *&`; no + // conversion takes place, `reference_type` is + // `some_namespace::some_struct *&`. + auto C = concat(V0, V1); + static_assert( + std::is_same_v); + EXPECT_THAT(C, ElementsAre(&S0, &S1, &S1)); + // `reference_type` should still allow container modification. + for (auto &i : C) + if (i == &S0) + i = nullptr; + EXPECT_THAT(C, ElementsAre(nullptr, &S1, &S1)); +} + +TEST(STLExtrasTest, ConcatRangePtrToDerivedClass) { + some_namespace::some_struct S0{}; + some_namespace::derives_from_some_struct S1{}; + SmallVector V0{&S0}; + SmallVector V1{&S1, &S1}; + + // Dereferencing all iterators yields different (but convertible types); + // conversion takes place, `reference_type` is + // `some_namespace::some_struct *`. + auto C = concat(V0, V1); + static_assert( + std::is_same_v); + EXPECT_THAT(C, + ElementsAre(&S0, static_cast(&S1), + static_cast(&S1))); +} + TEST(STLExtrasTest, MakeFirstSecondRangeADL) { // Make sure that we use the `begin`/`end` functions from `some_namespace`, // using ADL.