From 93a376047ecd49637fff2f21249f7e2d1593a365 Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Tue, 29 Apr 2025 13:19:16 +0100 Subject: [PATCH 1/7] [ArrayRef] Add constructor from iterator_range (NFC). 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. --- llvm/include/llvm/ADT/ArrayRef.h | 13 +++++++++++++ llvm/unittests/ADT/ArrayRefTest.cpp | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+) 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 from iterator_range. This uses SFINAE + /// to ensure that this is only used for iterator ranges of random access + /// iterators that can be converted. + template + ArrayRef(const iterator_range &Range, + std::enable_if_t:: + iterator_category>::value && + std::is_convertible::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 A1{{42, -5, 0, 1000000, -1000000}}; + ArrayRef 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 A1{{42, -5, 0, 1000000, -1000000}}; + ArrayRef 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>, "trivially copyable"); From 4105fd1387931bc71fa4213f612a1caea28fb493 Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Tue, 29 Apr 2025 17:40:23 +0100 Subject: [PATCH 2/7] !fixup address latest comments, thanks! --- llvm/include/llvm/ADT/ArrayRef.h | 17 +++++++++-------- llvm/unittests/ADT/ArrayRefTest.cpp | 6 ++---- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index f2fc7b636a8f5..47ba9fe3a23e3 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -152,14 +152,15 @@ namespace llvm { /// Construct an ArrayRef from iterator_range. This uses SFINAE /// to ensure that this is only used for iterator ranges of random access /// iterators that can be converted. - template - ArrayRef(const iterator_range &Range, - std::enable_if_t:: - iterator_category>::value && - std::is_convertible::value, - void> * = nullptr) + template &>() + .begin())>::iterator_category>::value && + std::is_convertible_v>> + ArrayRef(const iterator_range &Range) : Data(Range.begin()), Length(llvm::size(Range)) {} /// @} diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp index 128efbea813d2..098a0910b6cb8 100644 --- a/llvm/unittests/ADT/ArrayRefTest.cpp +++ b/llvm/unittests/ADT/ArrayRefTest.cpp @@ -260,9 +260,8 @@ TEST(ArrayRefTest, ArrayRefFromIteratorRange) { ArrayRef A2 = make_range(A1.begin(), A1.end()); EXPECT_EQ(A1.size(), A2.size()); - for (std::size_t i = 0; i < A1.size(); ++i) { + for (std::size_t i = 0; i < A1.size(); ++i) EXPECT_EQ(A1[i], A2[i]); - } } TEST(ArrayRefTest, ArrayRefFromIteratorConstRange) { @@ -270,9 +269,8 @@ TEST(ArrayRefTest, ArrayRefFromIteratorConstRange) { ArrayRef A2 = make_range(A1.begin(), A1.end()); EXPECT_EQ(A1.size(), A2.size()); - for (std::size_t i = 0; i < A1.size(); ++i) { + for (std::size_t i = 0; i < A1.size(); ++i) EXPECT_EQ(A1[i], A2[i]); - } } static_assert(std::is_trivially_copyable_v>, From da7f27522c920551140599c833f421dde53d201c Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Tue, 29 Apr 2025 17:43:40 +0100 Subject: [PATCH 3/7] !fixup enable_if --- llvm/include/llvm/ADT/ArrayRef.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index 47ba9fe3a23e3..c380f71832d13 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -153,7 +153,7 @@ namespace llvm { /// to ensure that this is only used for iterator ranges of random access /// iterators that can be converted. template Date: Tue, 29 Apr 2025 19:18:51 +0100 Subject: [PATCH 4/7] !fixup simplify SFINAE check --- llvm/include/llvm/ADT/ArrayRef.h | 13 +++---------- llvm/unittests/ADT/ArrayRefTest.cpp | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index c380f71832d13..4f5567ecc55b7 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -150,16 +150,9 @@ namespace llvm { : Data(Vec.data()), Length(Vec.size()) {} /// Construct an ArrayRef from iterator_range. This uses SFINAE - /// to ensure that this is only used for iterator ranges of random access - /// iterators that can be converted. - template &>() - .begin())>::iterator_category>::value && - std::is_convertible_v>> + /// to ensure that this is only used for iterator ranges over plain pointer + /// iterators. + template >> ArrayRef(const iterator_range &Range) : Data(Range.begin()), Length(llvm::size(Range)) {} diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp index 098a0910b6cb8..f6f36e5a881e1 100644 --- a/llvm/unittests/ADT/ArrayRefTest.cpp +++ b/llvm/unittests/ADT/ArrayRefTest.cpp @@ -255,6 +255,20 @@ TEST(ArrayRefTest, ArrayRefFromStdArray) { } } +struct TestRandomAccessIterator { + using iterator_category = std::random_access_iterator_tag; +}; + +static_assert( + !std::is_constructible, + iterator_range>::value, + "cannot construct from iterator range with non-pointer iterator"); +static_assert(!std::is_constructible, iterator_range>::value, + "cannot construct from iterator range with non-pointer iterator"); +static_assert( + std::is_constructible, iterator_range>::value, + "should be able to construct ArrayRef from iterator_range over pointers"); + TEST(ArrayRefTest, ArrayRefFromIteratorRange) { std::array A1{{42, -5, 0, 1000000, -1000000}}; ArrayRef A2 = make_range(A1.begin(), A1.end()); From d6e3f8d8f827ac8d2c03ca6ae4b53c314b4b6486 Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Wed, 30 Apr 2025 10:09:31 +0100 Subject: [PATCH 5/7] !fixup is_same_v -> is_convertible_v, is_constructible_v --- llvm/include/llvm/ADT/ArrayRef.h | 3 ++- llvm/unittests/ADT/ArrayRefTest.cpp | 16 ++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index 4f5567ecc55b7..1bb44806d223c 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -152,7 +152,8 @@ namespace llvm { /// Construct an ArrayRef from iterator_range. This uses SFINAE /// to ensure that this is only used for iterator ranges over plain pointer /// iterators. - template >> + template >> ArrayRef(const iterator_range &Range) : Data(Range.begin()), Length(llvm::size(Range)) {} diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp index f6f36e5a881e1..017193d5eaa4b 100644 --- a/llvm/unittests/ADT/ArrayRefTest.cpp +++ b/llvm/unittests/ADT/ArrayRefTest.cpp @@ -259,14 +259,13 @@ struct TestRandomAccessIterator { using iterator_category = std::random_access_iterator_tag; }; -static_assert( - !std::is_constructible, - iterator_range>::value, - "cannot construct from iterator range with non-pointer iterator"); -static_assert(!std::is_constructible, iterator_range>::value, +static_assert(!std::is_constructible_v< + ArrayRef, iterator_range>, + "cannot construct from iterator range with non-pointer iterator"); +static_assert(!std::is_constructible_v, iterator_range>, "cannot construct from iterator range with non-pointer iterator"); static_assert( - std::is_constructible, iterator_range>::value, + std::is_constructible_v, iterator_range>, "should be able to construct ArrayRef from iterator_range over pointers"); TEST(ArrayRefTest, ArrayRefFromIteratorRange) { @@ -276,6 +275,11 @@ TEST(ArrayRefTest, ArrayRefFromIteratorRange) { EXPECT_EQ(A1.size(), A2.size()); for (std::size_t i = 0; i < A1.size(); ++i) EXPECT_EQ(A1[i], A2[i]); + + ArrayRef A3 = make_range(A1.begin(), A1.end()); + EXPECT_EQ(A1.size(), A3.size()); + for (std::size_t i = 0; i < A1.size(); ++i) + EXPECT_EQ(A1[i], A3[i]); } TEST(ArrayRefTest, ArrayRefFromIteratorConstRange) { From 9d4410a1f2ab994b77674e096de096362eb4401c Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Wed, 30 Apr 2025 10:14:51 +0100 Subject: [PATCH 6/7] !fixup add astatic assert ArrayRef from ArrayRef. --- llvm/unittests/ADT/ArrayRefTest.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp index 017193d5eaa4b..8de75beeef19c 100644 --- a/llvm/unittests/ADT/ArrayRefTest.cpp +++ b/llvm/unittests/ADT/ArrayRefTest.cpp @@ -264,6 +264,11 @@ static_assert(!std::is_constructible_v< "cannot construct from iterator range with non-pointer iterator"); static_assert(!std::is_constructible_v, iterator_range>, "cannot construct from iterator range with non-pointer iterator"); +static_assert( + !std::is_constructible_v, iterator_range>, + "cannot construct ArrayRef with non-const elements from const iterator " + "range"); + static_assert( std::is_constructible_v, iterator_range>, "should be able to construct ArrayRef from iterator_range over pointers"); From e503b2ef57712f8505deddb2bc0e000e699ffa0c Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Wed, 30 Apr 2025 11:32:05 +0100 Subject: [PATCH 7/7] !fixup forbid creating ArrayRef from iterator_range --- llvm/include/llvm/ADT/ArrayRef.h | 4 ++-- llvm/unittests/ADT/ArrayRefTest.cpp | 19 ++++++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h index 1bb44806d223c..1f2433b9a7667 100644 --- a/llvm/include/llvm/ADT/ArrayRef.h +++ b/llvm/include/llvm/ADT/ArrayRef.h @@ -152,8 +152,8 @@ namespace llvm { /// Construct an ArrayRef from iterator_range. This uses SFINAE /// to ensure that this is only used for iterator ranges over plain pointer /// iterators. - template >> + template >> ArrayRef(const iterator_range &Range) : Data(Range.begin()), Length(llvm::size(Range)) {} diff --git a/llvm/unittests/ADT/ArrayRefTest.cpp b/llvm/unittests/ADT/ArrayRefTest.cpp index 8de75beeef19c..a9d682c57f7a0 100644 --- a/llvm/unittests/ADT/ArrayRefTest.cpp +++ b/llvm/unittests/ADT/ArrayRefTest.cpp @@ -264,14 +264,31 @@ static_assert(!std::is_constructible_v< "cannot construct from iterator range with non-pointer iterator"); static_assert(!std::is_constructible_v, iterator_range>, "cannot construct from iterator range with non-pointer iterator"); + +class TestBase {}; + +class TestDerived : public TestBase {}; + +static_assert( + !std::is_constructible_v, iterator_range>, + "cannot construct ArrayRef with derived type"); +static_assert( + !std::is_constructible_v, iterator_range>, + "cannot construct ArrayRef base type"); +static_assert(!std::is_constructible_v, + iterator_range>, + "cannot construct ArrayRef pointer of base type"); + static_assert( !std::is_constructible_v, iterator_range>, "cannot construct ArrayRef with non-const elements from const iterator " "range"); - static_assert( std::is_constructible_v, iterator_range>, "should be able to construct ArrayRef from iterator_range over pointers"); +static_assert( + !std::is_constructible_v, iterator_range>, + "should be able to construct ArrayRef from iterator_range over pointers"); TEST(ArrayRefTest, ArrayRefFromIteratorRange) { std::array A1{{42, -5, 0, 1000000, -1000000}};