Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −7
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions llvm/include/llvm/ADT/STLExtras.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,13 +1032,17 @@ class concat_iterator

static constexpr bool ReturnsByValue =
!(std::is_reference_v<decltype(*std::declval<IterTs>())> && ...);
static constexpr bool ReturnsConvertiblePointer =
std::is_pointer_v<ValueT> &&
(std::is_convertible_v<decltype(*std::declval<IterTs>()), ValueT> && ...);

using reference_type =
typename std::conditional_t<ReturnsByValue, ValueT, ValueT &>;
typename std::conditional_t<ReturnsByValue || ReturnsConvertiblePointer,
ValueT, ValueT &>;
Comment on lines +1035 to +1041
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 *.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


using handle_type =
typename std::conditional_t<ReturnsByValue, std::optional<ValueT>,
ValueT *>;
using handle_type = typename std::conditional_t<
ReturnsConvertiblePointer, ValueT,
std::conditional_t<ReturnsByValue, std::optional<ValueT>, ValueT *>>;
Comment on lines +1035 to +1045
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍.


/// We store both the current and end iterators for each concatenated
/// sequence in a tuple of pairs.
Expand Down Expand Up @@ -1088,7 +1092,7 @@ class concat_iterator
if (Begin == End)
return {};

if constexpr (ReturnsByValue)
if constexpr (ReturnsByValue || ReturnsConvertiblePointer)
return *Begin;
else
return &*Begin;
Expand All @@ -1105,8 +1109,12 @@ class concat_iterator

// Loop over them, and return the first result we find.
for (auto &GetHelperFn : GetHelperFns)
if (auto P = (this->*GetHelperFn)())
return *P;
if (auto P = (this->*GetHelperFn)()) {
if constexpr (ReturnsConvertiblePointer)
return P;
else
return *P;
}

llvm_unreachable("Attempted to get a pointer from an end concat iterator!");
}
Expand Down
14 changes: 14 additions & 0 deletions llvm/unittests/ADT/STLExtrasTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,8 @@ 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();
}
Expand Down Expand Up @@ -532,6 +534,18 @@ TEST(STLExtrasTest, ConcatRangeADL) {
EXPECT_THAT(concat<const int>(S0, S1), ElementsAre(1, 2, 3, 4));
}

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)));
}

TEST(STLExtrasTest, MakeFirstSecondRangeADL) {
// Make sure that we use the `begin`/`end` functions from `some_namespace`,
// using ADL.
Expand Down