[ArrayRef] Add constructor from iterator_range<U*> (NFC). #137796
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-adt Author: Florian Hahn (fhahn) ChangesAdd a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. I will share a follow-up soon. Full diff: https://github.com/llvm/llvm-project/pull/137796.diff 2 Files Affected:
diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h
index a1317423cdd1a..f2fc7b636a8f5 100644
--- a/llvm/include/llvm/ADT/ArrayRef.h
+++ b/llvm/include/llvm/ADT/ArrayRef.h
@@ -149,6 +149,19 @@ namespace llvm {
* = nullptr)
: Data(Vec.data()), Length(Vec.size()) {}
+ /// Construct an ArrayRef<T> from iterator_range<U*>. This uses SFINAE
+ /// to ensure that this is only used for iterator ranges of random access
+ /// iterators that can be converted.
+ template <typename U>
+ ArrayRef(const iterator_range<U *> &Range,
+ std::enable_if_t<std::is_base_of<std::random_access_iterator_tag,
+ typename std::iterator_traits<
+ decltype(Range.begin())>::
+ iterator_category>::value &&
+ std::is_convertible<U *, T const *>::value,
+ void> * = nullptr)
+ : Data(Range.begin()), Length(llvm::size(Range)) {}
+
/// @}
/// @name Simple Operations
/// @{
diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp
index fb25ee19c0b20..128efbea813d2 100644
--- a/llvm/unittests/ADT/ArrayRefTest.cpp
+++ b/llvm/unittests/ADT/ArrayRefTest.cpp
@@ -255,6 +255,26 @@ TEST(ArrayRefTest, ArrayRefFromStdArray) {
}
}
+TEST(ArrayRefTest, ArrayRefFromIteratorRange) {
+ std::array<int, 5> A1{{42, -5, 0, 1000000, -1000000}};
+ ArrayRef<int> A2 = make_range(A1.begin(), A1.end());
+
+ EXPECT_EQ(A1.size(), A2.size());
+ for (std::size_t i = 0; i < A1.size(); ++i) {
+ EXPECT_EQ(A1[i], A2[i]);
+ }
+}
+
+TEST(ArrayRefTest, ArrayRefFromIteratorConstRange) {
+ std::array<const int, 5> A1{{42, -5, 0, 1000000, -1000000}};
+ ArrayRef<const int> A2 = make_range(A1.begin(), A1.end());
+
+ EXPECT_EQ(A1.size(), A2.size());
+ for (std::size_t i = 0; i < A1.size(); ++i) {
+ EXPECT_EQ(A1[i], A2[i]);
+ }
+}
+
static_assert(std::is_trivially_copyable_v<ArrayRef<int>>,
"trivially copyable");
|
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Apr 29, 2025
Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on llvm#137796.
fhahn
commented
Apr 29, 2025
artagnon
reviewed
Apr 29, 2025
llvm/include/llvm/ADT/ArrayRef.h
Outdated
| /// iterators that can be converted. | ||
| template <typename U> | ||
| ArrayRef(const iterator_range<U *> &Range, | ||
| std::enable_if_t<std::is_base_of<std::random_access_iterator_tag, |
There was a problem hiding this comment.
Use enable_if instead of enable_if_it, and put it as a template parameter?
llvm/include/llvm/ADT/ArrayRef.h
Outdated
Comment on lines
158
to
160
| typename std::iterator_traits< | ||
| decltype(Range.begin())>:: | ||
| iterator_category>::value && |
There was a problem hiding this comment.
Make this also a template parameter?
There was a problem hiding this comment.
move to template<> is that what you had in mind?
llvm/include/llvm/ADT/ArrayRef.h
Outdated
| typename std::iterator_traits< | ||
| decltype(Range.begin())>:: | ||
| iterator_category>::value && | ||
| std::is_convertible<U *, T const *>::value, |
There was a problem hiding this comment.
I think there is a is_convertible_v?
kuhar
reviewed
Apr 29, 2025
fhahn
force-pushed
the
arrayref-from-iterator-range
branch
from
5f88e72 to
2dcafd4
Compare
dwblaikie
reviewed
Apr 29, 2025
llvm/include/llvm/ADT/ArrayRef.h
Outdated
| /// to ensure that this is only used for iterator ranges of random access | ||
| /// iterators that can be converted. | ||
| template <typename U, | ||
| typename = std::enable_if< |
There was a problem hiding this comment.
Probably still worth using std::enable_if_t here, but without explicitly specifying a type parameter (the default is void, so that's fine) - if you are using std::enable_if then I think you have to add the ::type at the end (that's where the SFINAE is - std::enable_if<false> has no nested type member, but std::enable_if<true> does)
Speaking of which - perhaps you could use some static_asserts to check that this SFINAE is rejecting the relevant cases in this condition?
Also - perhaps you could explain more how this condition works? It's not clear to me it does what we want/can detect contiguous sequences.
There was a problem hiding this comment.
I think we can drop the typename =.
I tried but it complained.
Probably still worth using std::enable_if_t here, but without explicitly specifying a type parameter (the default is void, so that's fine) - if you are using std::enable_if then I think you have to add the ::type at the end (that's where the SFINAE is - std::enable_if has no nested type member, but std::enable_if does)
thanks, switched back to std::enable_if_t. The original checks were more complex and maybe too lax than needed; I think we are only guaranteed to iterate over a continous range, if the iterator is a plain pointer (e.g. int * as in the test case or something like int **.
The new check just enforces that the iterator is same as T *. I think that should be sufficient?
I also added some static_assert to check some invalid combinations.
artagnon
reviewed
Apr 29, 2025
llvm/include/llvm/ADT/ArrayRef.h
Outdated
| /// iterators that can be converted. | ||
| template <typename U, | ||
| typename = std::enable_if< | ||
| std::is_base_of< |
llvm/include/llvm/ADT/ArrayRef.h
Outdated
| /// to ensure that this is only used for iterator ranges of random access | ||
| /// iterators that can be converted. | ||
| template <typename U, | ||
| typename = std::enable_if< |
There was a problem hiding this comment.
I think we can drop the typename =.
llvm/include/llvm/ADT/ArrayRef.h
Outdated
Comment on lines
159
to
161
| typename std::iterator_traits< | ||
| decltype(std::declval<const iterator_range<U *> &>() | ||
| .begin())>::iterator_category>::value && |
There was a problem hiding this comment.
So what I meant is that we can factor this typename into a new template parameter, say IT, and then use IT here to keep this template parameter readable?
llvm/include/llvm/ADT/ArrayRef.h
Outdated
Comment on lines
160
to
161
| decltype(std::declval<const iterator_range<U *> &>() | ||
| .begin())>::iterator_category>::value && |
There was a problem hiding this comment.
I don't think we need this declval, but I could be wrong: does iterator_traits<iterator<U *>>::iterator_category not work? Why use iterator_range::begin(), when we already know what the type of the begin is?
There was a problem hiding this comment.
This is now gone.
We now just need the type of the iterator, which we can get from the template argument already, thanks
fhahn
force-pushed
the
arrayref-from-iterator-range
branch
from
bdb3080 to
0e21ecb
Compare
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Apr 29, 2025
Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on llvm#137796.
|
(seems about right to me, I'll leave it to @artagnon for the final approval, unless something contentious comes up (if so, do tag me/reach out, happy to weigh in/help resolve)) |
artagnon
approved these changes
Apr 30, 2025
llvm/unittests/ADT/ArrayRefTest.cpp
Outdated
| }; | ||
|
|
||
| static_assert( | ||
| !std::is_constructible<ArrayRef<int>, |
artagnon
reviewed
Apr 30, 2025
llvm/include/llvm/ADT/ArrayRef.h
Outdated
| /// Construct an ArrayRef<T> from iterator_range<U*>. This uses SFINAE | ||
| /// to ensure that this is only used for iterator ranges over plain pointer | ||
| /// iterators. | ||
| template <typename U, typename = std::enable_if_t<std::is_same_v<U *, T *>>> |
There was a problem hiding this comment.
Sorry about the thinko, but why is template parameter U needed if std::is_same_iv<U *, T *>? Doesn't that imply that U = T?
There was a problem hiding this comment.
Yep, this should use is_convertible I think, to support creating ArrayRef<const X> from iterator_range<X *>, also added a test
There was a problem hiding this comment.
Not 100% sure if is_convertible is the best thing to do here: maybe is_same_v<U, remove_const_t<T>> is better?
There was a problem hiding this comment.
is_convertible will also allow creating a type of base class from a derived class, and I think we want to forbid such things in the construction.
There was a problem hiding this comment.
Unfortunately is_same_v<U, remove_const_t<T>> isn't sufficient to support pointer cases with different const-ness directly .
I updated the is_convertible_v check to add another level of indirection, which should forbids construction a ArrayRef for a non-pointer base from an iterator of derived*, the only case that was previously allowed. This is similar to how std::vector and SmallVectorImpl are handled above. Added a few more static asserts to the test
Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. I will share a follow-up soon.
fhahn
force-pushed
the
arrayref-from-iterator-range
branch
from
0e21ecb to
d6e3f8d
Compare
artagnon
approved these changes
Apr 30, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/22803 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/16019 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/19260 Here is the relevant piece of the build log for the reference |
|
Windows failure to build unit tests looks related to std::array, should be fixed in 0d6c9f3 |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. To be used in llvm#137798. PR: llvm#137796
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…FC) (llvm#137798) Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on llvm#137796. PR: llvm#137798
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. To be used in llvm#137798. PR: llvm#137796
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…FC) (llvm#137798) Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on llvm#137796. PR: llvm#137798
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. To be used in llvm#137798. PR: llvm#137796
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…FC) (llvm#137798) Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on llvm#137796. PR: llvm#137798
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 6, 2025
…(#137796) Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. To be used in llvm/llvm-project#137798. PR: llvm/llvm-project#137796
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 6, 2025
…in ctors (NFC) (#137798) Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on llvm/llvm-project#137796. PR: llvm/llvm-project#137798
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. To be used in llvm#137798. PR: llvm#137796
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
…FC) (llvm#137798) Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on llvm#137796. PR: llvm#137798
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted. This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. To be used in llvm#137798. PR: llvm#137796
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
…FC) (llvm#137798) Now that there is an ArrayRef constructor taking iterator_ranges, use it consistently to slightly simplify code. Depends on llvm#137796. PR: llvm#137798
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a new constructor to ArrayRef that takes an iterator_range with a random access iterator that can be converted.
This can help to avoid creating unnecessary iterator_ranges for types where an ArrayRef can already be constructed. I will share a follow-up soon.