[ADT] Fix llvm::concat_iterator for ValueT == common_base_class *

[jalopezg-git]

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. In particular, the case below was not working before this patch, but I see no particular reason why it shouldn't be supported.

namespace some_namespace {
struct some_struct {
  std::vector<int> data;
  std::string swap_val;
};

struct derives_from_some_struct : some_struct {
};
} // namespace some_namespace

TEST(STLExtrasTest, ConcatRangePtrToDerivedClass) {
  some_namespace::some_struct S0{};
  some_namespace::derives_from_some_struct S1{};
  SmallVector<some_namespace::some_struct *> V0{&S0};
  SmallVector<some_namespace::derives_from_some_struct *> V1{&S1, &S1};

  // Use concat over ranges of pointers to different (but related) types.
  EXPECT_THAT(concat<some_namespace::some_struct *>(V0, V1),
              ElementsAre(&S0, static_cast<some_namespace::some_struct *>(&S1),
                          static_cast<some_namespace::some_struct *>(&S1)));
}

Per https://en.cppreference.com/w/cpp/language/implicit_conversion.html, Sec. Pointer conversions,

A prvalue ptr of type “pointer to (possibly cv-qualified) Derived” can be converted to a prvalue of type “pointer to (possibly cv-qualified) Base”, where Base is a base class of Derived, and Derived is a complete class type.

I.e. conversion shall be possible, but the former implementation of llvm::concat_iterator forbids that, given that handle_type is pointer-to-pointer type, which is not convertible.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Javier Lopez-Gomez (jalopezg-git)

Changes

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.

In particular, the case below was not working before this patch, but I see no particular reason why it shouldn't be supported.

namespace some_namespace {
struct some_struct {
  std::vector<int> data;
  std::string swap_val;
};

struct derives_from_some_struct : some_struct {
};
} // namespace some_namespace

TEST(STLExtrasTest, ConcatRangePtrToDerivedClass) {
  auto S0 = std::make_unique<some_namespace::some_struct>();
  auto S1 = std::make_unique<some_namespace::derives_from_some_struct>();
  SmallVector<some_namespace::some_struct *> V0{S0.get()};
  SmallVector<some_namespace::derives_from_some_struct *> V1{S1.get(), S1.get()};

  // Use concat over ranges of pointers to different (but related) types.
  EXPECT_THAT(concat<some_namespace::some_struct *>(V0, V1),
             ElementsAre(S0.get(),
                         static_cast<some_namespace::some_struct *>(S1.get()),
                         static_cast<some_namespace::some_struct *>(S1.get())));
}

---

Full diff: https://github.com/llvm/llvm-project/pull/144744.diff

2 Files Affected:

• (modified) llvm/include/llvm/ADT/STLExtras.h (+6-5)
• (modified) llvm/unittests/ADT/STLExtrasTest.cpp (+16)

diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index eea06cfb99ba2..951da522a8aa2 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -1030,14 +1030,15 @@ class concat_iterator
                                   std::forward_iterator_tag, ValueT> {
   using BaseT = typename concat_iterator::iterator_facade_base;
 
-  static constexpr bool ReturnsByValue =
-      !(std::is_reference_v<decltype(*std::declval<IterTs>())> && ...);
+  static constexpr bool ReturnsValueOrPointer =
+      !(std::is_reference_v<decltype(*std::declval<IterTs>())> && ...)
+      || (std::is_pointer_v<IterTs> && ...);
 
   using reference_type =
-      typename std::conditional_t<ReturnsByValue, ValueT, ValueT &>;
+      typename std::conditional_t<ReturnsValueOrPointer, ValueT, ValueT &>;
 
   using handle_type =
-      typename std::conditional_t<ReturnsByValue, std::optional<ValueT>,
+      typename std::conditional_t<ReturnsValueOrPointer, std::optional<ValueT>,
                                   ValueT *>;
 
   /// We store both the current and end iterators for each concatenated
@@ -1088,7 +1089,7 @@ class concat_iterator
     if (Begin == End)
       return {};
 
-    if constexpr (ReturnsByValue)
+    if constexpr (ReturnsValueOrPointer)
       return *Begin;
     else
       return &*Begin;
diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp
index 286cfa745fd14..0e6b040a08f4a 100644
--- a/llvm/unittests/ADT/STLExtrasTest.cpp
+++ b/llvm/unittests/ADT/STLExtrasTest.cpp
@@ -398,6 +398,9 @@ struct some_struct {
   std::string swap_val;
 };
 
+struct derives_from_some_struct : some_struct {
+};
+
 std::vector<int>::const_iterator begin(const some_struct &s) {
   return s.data.begin();
 }
@@ -532,6 +535,19 @@ TEST(STLExtrasTest, ConcatRangeADL) {
   EXPECT_THAT(concat<const int>(S0, S1), ElementsAre(1, 2, 3, 4));
 }
 
+TEST(STLExtrasTest, ConcatRangePtrToDerivedClass) {
+  auto S0 = std::make_unique<some_namespace::some_struct>();
+  auto S1 = std::make_unique<some_namespace::derives_from_some_struct>();
+  SmallVector<some_namespace::some_struct *> V0{S0.get()};
+  SmallVector<some_namespace::derives_from_some_struct *> V1{S1.get(), S1.get()};
+
+  // Use concat over ranges of pointers to different (but related) types.
+  EXPECT_THAT(concat<some_namespace::some_struct *>(V0, V1),
+	      ElementsAre(S0.get(),
+			  static_cast<some_namespace::some_struct *>(S1.get()),
+			  static_cast<some_namespace::some_struct *>(S1.get())));
+}
+
 TEST(STLExtrasTest, MakeFirstSecondRangeADL) {
   // Make sure that we use the `begin`/`end` functions from `some_namespace`,
   // using ADL.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[kuhar]

This is getting complicated with the need for a nested condition -- could we move this to a new type trait struct or find some other way to simplify the code?

[jalopezg-git]

On second thoughts, I cannot see an obvious way to simplify this conditional type; unfortunately, defining a type trait struct would span over more lines (and I don't think it pays off given that this type is only used by a couple of private member functions).

I am not sure it is much clearer, but if it helps, it can be trivially split as

  using wrap_type =
      typename std::conditional_t<ReturnsByValue, std::optional<ValueT>, ValueT *>;
  using handle_type = typename std::conditional_t<ReturnsConvertiblePointer, ValueT, wrap_type>;

Any preference? Other than that, I think the PR is ready; build passing 👍.

[kuhar]

I took a closer look and I think I understand what the underlying issue was: with mixed pointer types, reference/handle type can no longer be T*& / T** because while it's fine to cast derived pointers to T*, we can't take an address off them using that base type.

I wonder if we should tighten this check to make sure that at least one input range type doesn't match the ValueT pointer type, and with that having them be returned via the same logic as ReturnsByValue doesn't sounds too bad to me. WDYT?

[kuhar]

Doesn't this change the reference type in the case where all pointer types are the same? Prior to this PR, the reference_type used to be T *&, now it will become T *.

[kuhar]

Similar for handle_type, doesn't this change the type from T ** to T *?