[libcxx][test] std::array::iterator are not pointers by C++ standard #70729
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
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Duo Wang (wdunicornpro) ChangesThis is to modify a list of libcxx tests written under the assumption that iterators for std::array, std::string_view, and std::string are pointers. The motivation for this PR is to make the tests more universal and potentially being used to test other C++ standard library implementations, for example microsoft/STL. I can confirm that this patch makes a number of tests compatible with microsoft STL: This is not a complete list of such incompatibilities, but I am hoping this will start a discussion about whether we are open to accept such changes. Patch is 72.76 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/70729.diff 40 Files Affected:
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
index bc9c2788293cd48..2dd7350b1c35cf8 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
@@ -128,8 +128,8 @@ constexpr bool test() {
{ // check that an iterator is returned with a borrowing range
std::array in{1, 2, 3, 4};
std::array<int, 4> out;
- std::same_as<std::ranges::in_out_result<int*, int*>> auto ret = std::ranges::copy(std::views::all(in), out.data());
- assert(ret.in == in.data() + 4);
+ std::same_as<std::ranges::in_out_result<std::array<int, 4>::iterator, int*>> auto ret = std::ranges::copy(std::views::all(in), out.data());
+ assert(ret.in == in.end());
assert(ret.out == out.data() + 4);
assert(in == out);
}
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp
index 184008c3e2fd0ff..feca8fd3be858bb 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp
@@ -247,9 +247,9 @@ constexpr bool test() {
{ // check that an iterator is returned with a borrowing range
std::array in {1, 2, 3, 4};
std::array<int, 4> out;
- std::same_as<std::ranges::in_out_result<int*, int*>> auto ret =
+ std::same_as<std::ranges::in_out_result<std::array<int, 4>::iterator, int*>> auto ret =
std::ranges::copy_backward(std::views::all(in), out.data() + out.size());
- assert(ret.in == in.data() + in.size());
+ assert(ret.in == in.end());
assert(ret.out == out.data());
assert(in == out);
}
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_n.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_n.pass.cpp
index 237d1ef115090c0..d2a2b7c4888308f 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_n.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_n.pass.cpp
@@ -53,7 +53,7 @@ constexpr void test_iterators() {
{ // check that an empty range works
std::array<int, 0> in;
std::array<int, 0> out;
- auto ret = std::ranges::copy_n(In(in.data()), in.size(), Out(out.begin()));
+ auto ret = std::ranges::copy_n(In(in.data()), in.size(), Out(out.data()));
assert(base(ret.in) == in.data());
assert(base(ret.out) == out.data());
}
@@ -103,7 +103,7 @@ constexpr bool test() {
};
std::array<CopyOnce, 4> in {};
std::array<CopyOnce, 4> out {};
- auto ret = std::ranges::copy_n(in.data(), in.size(), out.begin());
+ auto ret = std::ranges::copy_n(in.begin(), in.size(), out.begin());
assert(ret.in == in.end());
assert(ret.out == out.end());
assert(std::all_of(out.begin(), out.end(), [](const auto& e) { return e.copied; }));
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp
index 0697f4fc57d671f..1be5a1bb284aaae 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp
@@ -120,13 +120,13 @@ constexpr bool test() {
{
std::array<std::string, 10> a;
auto ret = std::ranges::fill(a.begin(), a.end(), "long long string so no SSO");
- assert(ret == a.data() + a.size());
+ assert(ret == a.begin() + a.size());
assert(std::all_of(a.begin(), a.end(), [](auto& s) { return s == "long long string so no SSO"; }));
}
{
std::array<std::string, 10> a;
auto ret = std::ranges::fill(a, "long long string so no SSO");
- assert(ret == a.data() + a.size());
+ assert(ret == a.begin() + a.size());
assert(std::all_of(a.begin(), a.end(), [](auto& s) { return s == "long long string so no SSO"; }));
}
}
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp
index aeff581c46cb517..a0d1473360a14e8 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp
@@ -275,9 +275,9 @@ constexpr bool test() {
{ // check that an iterator is returned with a borrowing range
std::array in {1, 2, 3, 4};
std::array<int, 4> out;
- std::same_as<std::ranges::in_out_result<int*, int*>> decltype(auto) ret =
+ std::same_as<std::ranges::in_out_result<std::array<int, 4>::iterator, int*>> decltype(auto) ret =
std::ranges::move(std::views::all(in), out.data());
- assert(ret.in == in.data() + 4);
+ assert(ret.in == in.end());
assert(ret.out == out.data() + 4);
assert(in == out);
}
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp
index 9219cbb1dd84d1f..47cf178636ad130 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move_backward.pass.cpp
@@ -261,9 +261,9 @@ constexpr bool test() {
{ // check that an iterator is returned with a borrowing range
std::array in {1, 2, 3, 4};
std::array<int, 4> out;
- std::same_as<std::ranges::in_out_result<int*, int*>> auto ret =
+ std::same_as<std::ranges::in_out_result<std::array<int, 4>::iterator, int*>> auto ret =
std::ranges::move_backward(std::views::all(in), out.data() + out.size());
- assert(ret.in == in.data() + in.size());
+ assert(ret.in == in.end());
assert(ret.out == out.data());
assert(in == out);
}
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp
index 7b5b80dd8aaf819..af9a72da71a9985 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.partitions/ranges_partition_copy.pass.cpp
@@ -132,7 +132,7 @@ constexpr void test_one(std::array<int, N1> input, Pred pred, std::array<int, N2
std::array<int, N3> out2;
std::same_as<ResultT> decltype(auto) result = std::ranges::partition_copy(
- Iter(begin), Sent(Iter(end)), OutIter1(out1.begin()), OutIter2(out2.begin()), pred);
+ Iter(begin), Sent(Iter(end)), OutIter1(out1.data()), OutIter2(out2.data()), pred);
assert(base(result.in) == input.data() + input.size());
assert(base(result.out1) == out1.data() + expected_true.size());
@@ -148,7 +148,7 @@ constexpr void test_one(std::array<int, N1> input, Pred pred, std::array<int, N2
std::array<int, N3> out2;
std::same_as<ResultT> decltype(auto) result = std::ranges::partition_copy(
- range, OutIter1(out1.begin()), OutIter2(out2.begin()), pred);
+ range, OutIter1(out1.data()), OutIter2(out2.data()), pred);
assert(base(result.in) == input.data() + input.size());
assert(base(result.out1) == out1.data() + expected_true.size());
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.random.sample/ranges_sample.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.random.sample/ranges_sample.pass.cpp
index 69db960ff362f6b..a5178cb2eee132b 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.random.sample/ranges_sample.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.random.sample/ranges_sample.pass.cpp
@@ -164,7 +164,7 @@ void test_one(std::array<int, N> in, std::size_t n, Gen gen) {
auto begin = Iter(in.data());
auto end = Sent(Iter(in.data() + in.size()));
std::array<int, N> output;
- auto out = Out(output.begin());
+ auto out = Out(output.data());
std::same_as<Out> decltype(auto) result = std::ranges::sample(
std::move(begin), std::move(end), std::move(out), n, gen);
@@ -177,7 +177,7 @@ void test_one(std::array<int, N> in, std::size_t n, Gen gen) {
auto begin = Iter(in.data());
auto end = Sent(Iter(in.data() + in.size()));
std::array<int, N> output;
- auto out = Out(output.begin());
+ auto out = Out(output.data());
std::same_as<Out> decltype(auto) result = std::ranges::sample(std::ranges::subrange(
std::move(begin), std::move(end)), std::move(out), n, gen);
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove.pass.cpp
index 7836dede7342ae9..7778fe8ce7cfdd0 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove.pass.cpp
@@ -65,7 +65,7 @@ constexpr void test(Data<N, M> d) {
assert(base(ret.begin()) == input.data() + M);
assert(base(ret.end()) == input.data() + N);
- assert(std::ranges::equal(input.begin(), base(ret.begin()), d.expected.begin(), d.expected.end()));
+ assert(std::ranges::equal(input.data(), base(ret.begin()), d.expected.begin(), d.expected.end()));
}
{ // range overload
@@ -76,7 +76,7 @@ constexpr void test(Data<N, M> d) {
assert(base(ret.begin()) == input.data() + M);
assert(base(ret.end()) == input.data() + N);
- assert(std::ranges::equal(base(input.begin()), base(ret.begin()), d.expected.begin(), d.expected.end()));
+ assert(std::ranges::equal(input.data(), base(ret.begin()), d.expected.begin(), d.expected.end()));
}
}
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp
index 1db4f171b91072b..39f82a813333fcf 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp
@@ -85,7 +85,7 @@ constexpr void test(Data<N, M> d) {
assert(base(ret.begin()) == input.data() + M);
assert(base(ret.end()) == input.data() + N);
- assert(std::ranges::equal(base(input.begin()), base(ret.begin()), d.expected.begin(), d.expected.end()));
+ assert(std::ranges::equal(input.data(), base(ret.begin()), d.expected.begin(), d.expected.end()));
}
}
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp
index 4895ef59fc4eb00..a8d69b2832b4626 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.swap/ranges.swap_ranges.pass.cpp
@@ -65,7 +65,7 @@ constexpr void test_range() {
std::array r2 = {4, 5, 6};
- std::same_as<std::ranges::in_in_result<int*, int*>> auto r = std::ranges::swap_ranges(r1, r2);
+ std::same_as<std::ranges::in_in_result<std::array<int, 3>::iterator, std::array<int, 3>::iterator>> auto r = std::ranges::swap_ranges(r1, r2);
assert(r.in1 == r1.end());
assert(r.in2 == r2.end());
@@ -146,7 +146,7 @@ constexpr void test_iterators() {
constexpr void test_rval_range() {
{
- using Expected = std::ranges::swap_ranges_result<int*, std::ranges::dangling>;
+ using Expected = std::ranges::swap_ranges_result<std::array<int, 3>::iterator, std::ranges::dangling>;
std::array<int, 3> r = {1, 2, 3};
std::same_as<Expected> auto a = std::ranges::swap_ranges(r, std::array{4, 5, 6});
assert((r == std::array{4, 5, 6}));
@@ -154,7 +154,7 @@ constexpr void test_rval_range() {
}
{
std::array<int, 3> r = {1, 2, 3};
- using Expected = std::ranges::swap_ranges_result<std::ranges::dangling, int*>;
+ using Expected = std::ranges::swap_ranges_result<std::ranges::dangling, std::array<int, 3>::iterator>;
std::same_as<Expected> auto b = std::ranges::swap_ranges(std::array{4, 5, 6}, r);
assert((r == std::array{4, 5, 6}));
assert(b.in2 == r.begin() + 3);
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.unique/ranges_unique_copy.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.unique/ranges_unique_copy.pass.cpp
index 801385cf691ad13..b84600b92c2b291 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.unique/ranges_unique_copy.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.unique/ranges_unique_copy.pass.cpp
@@ -186,7 +186,7 @@ constexpr void testUniqueCopyImpl(std::array<int, N1> in, std::array<int, N2> ex
{
std::array<int, N2> out;
std::same_as<std::ranges::unique_copy_result<InIter, OutIter>> decltype(auto) result =
- std::ranges::unique_copy(InIter{in.data()}, Sent{InIter{in.data() + in.size()}}, OutIter{out.begin()});
+ std::ranges::unique_copy(InIter{in.data()}, Sent{InIter{in.data() + in.size()}}, OutIter{out.data()});
assert(std::ranges::equal(out, expected));
assert(base(result.in) == in.data() + in.size());
assert(base(result.out) == out.data() + out.size());
@@ -197,7 +197,7 @@ constexpr void testUniqueCopyImpl(std::array<int, N1> in, std::array<int, N2> ex
std::array<int, N2> out;
std::ranges::subrange r{InIter{in.data()}, Sent{InIter{in.data() + in.size()}}};
std::same_as<std::ranges::unique_copy_result<InIter, OutIter>> decltype(auto) result =
- std::ranges::unique_copy(r, OutIter{out.begin()});
+ std::ranges::unique_copy(r, OutIter{out.data()});
assert(std::ranges::equal(out, expected));
assert(base(result.in) == in.data() + in.size());
assert(base(result.out) == out.data() + out.size());
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp
index 3118ad7b1d475a7..33f583800ad2170 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/ranges.all_of.pass.cpp
@@ -93,7 +93,7 @@ constexpr void test_iterators() {
auto pred = [&](int) { ++predicateCount; return true; };
auto proj = [&](int i) { ++projectionCount; return i; };
std::array a = {9, 7, 5, 3};
- assert(std::ranges::all_of(It(a.begin()), Sent(It(a.end())), pred, proj));
+ assert(std::ranges::all_of(It(a.data()), Sent(It(a.data() + a.size())), pred, proj));
assert(predicateCount == 4);
assert(projectionCount == 4);
}
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.any_of/ranges.any_of.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.any_of/ranges.any_of.pass.cpp
index 3dbac4bbc4a546a..312f89f54cd723f 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.any_of/ranges.any_of.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.any_of/ranges.any_of.pass.cpp
@@ -93,7 +93,7 @@ constexpr void test_iterators() {
auto pred = [&](int) { ++predicateCount; return false; };
auto proj = [&](int i) { ++projectionCount; return i; };
std::array a = {9, 7, 5, 3};
- assert(!std::ranges::any_of(It(a.begin()), Sent(It(a.end())), pred, proj));
+ assert(!std::ranges::any_of(It(a.data()), Sent(It(a.data() + a.size())), pred, proj));
assert(predicateCount == 4);
assert(projectionCount == 4);
}
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.none_of/ranges.none_of.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.none_of/ranges.none_of.pass.cpp
index cae29c3733b8876..dc9cfa39cbd6808 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.none_of/ranges.none_of.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.none_of/ranges.none_of.pass.cpp
@@ -93,7 +93,7 @@ constexpr void test_iterators() {
auto pred = [&](int) { ++predicateCount; return false; };
auto proj = [&](int i) { ++projectionCount; return i; };
std::array a = {9, 7, 5, 3};
- assert(std::ranges::none_of(It(a.begin()), Sent(It(a.end())), pred, proj));
+ assert(std::ranges::none_of(It(a.data()), Sent(It(a.data() + a.size())), pred, proj));
assert(predicateCount == 4);
assert(projectionCount == 4);
}
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp
index 3f4ef7760c9f58e..27babf3ee58744f 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/mismatch/ranges_mismatch.pass.cpp
@@ -105,7 +105,7 @@ constexpr bool test() {
{ // test with a range
std::array<int, 5> a = {1, 2, 3, 4, 5};
std::array<int, 5> b = {1, 2, 3, 5, 4};
- using Expected = std::ranges::mismatch_result<int*, int*>;
+ using Expected = std::ranges::mismatch_result<std::array<int, 5>::iterator, std::array<int, 5>::iterator>;
std::same_as<Expected> auto ret = std::ranges::mismatch(a, b);
assert(ret.in1 == a.begin() + 3);
assert(ret.in2 == b.begin() + 3);
diff --git a/libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp b/libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp
index 5d8086df450abb8..77be3660ac436f3 100644
--- a/libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/make.heap/ranges_make_heap.pass.cpp
@@ -60,6 +60,13 @@ static_assert(!HasMakeHeapR<UncheckedRange<const int*>>); // Doesn't satisfy `so
template <std::size_t N, class T, class Iter>
constexpr void verify_heap(const std::array<T, N>& heapified, Iter last, std::array<T, N> expected) {
+ assert(heapified == expected);
+ assert(last == heapified.end());
+ assert(std::is_heap(heapified.begin(), heapified.end()));
+}
+
+template <std::size_t N, class T, class Iter>
+constexpr void verify_heap_iterator(const std::array<T, N>& heapified, Iter last, std::array<T, N> expected) {
assert(heapified == expected);
assert(base(last) == heapified.data() + heapified.size());
assert(std::is_heap(heapified.begin(), heapified.end()));
@@ -73,7 +80,7 @@ constexpr void test_one(const std::array<int, N> input, std::array<int, N> expec
auto e = Sent(Iter(heapified.data() + heapified.size()));
std::same_as<Iter> decltype(auto) last = std::ranges::make_heap(b, e);
- verify_heap(heapified, last, expected);
+ verify_heap_iterator(heapified, last, expected);
}
{ // (range) overload.
@@ -83,7 +90,7 @@ constexpr void test_one(const std::array<int, N> input, std::array<int, N> expec
auto range = std::ranges::subrange(b, e);
std::same_as<Iter> decltype(auto) last = std::ranges::make_heap(range);
- verify_heap(heapified, last, expected);
+ verify_heap_iterator(heapified, last, expected);
}
}
diff --git a/libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/ranges_pop_heap.pass.cpp b/libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/ranges_pop_heap.pass.cpp
index e190586fef813bb..de22a40d1cd7a96 100644
--- a/libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/ranges_pop_heap.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/pop.heap/ranges_pop_heap.pass.cpp
@@ -60,6 +60,14 @@ static_assert(!HasPopHeapR<UncheckedRange<const int*>>); // Doesn't satisfy `sor
template <std::size_t N, class T, class Iter>
constexpr void verify_heap(const std::array<T, N>& heapified, Iter last, std::array<T, N> expected) {
+ assert(heapified == expected);
+ assert(la...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
philnik777
requested changes
Oct 30, 2023
There was a problem hiding this comment.
We actually try to make the tests under test/std strictly conforming code, so this doesn't need any discussion. We're just not perfect (far from it) at spotting implementation-specific assumptions. Thanks for fixing these cases!
There are a few minor things. The rest looks pretty good.
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp
Outdated
Show resolved
Hide resolved
| @@ -60,6 +60,13 @@ static_assert(!HasMakeHeapR<UncheckedRange<const int*>>); // Doesn't satisfy `so | |||
|
|
|||
| template <std::size_t N, class T, class Iter> | |||
| constexpr void verify_heap(const std::array<T, N>& heapified, Iter last, std::array<T, N> expected) { | |||
| assert(heapified == expected); | |||
There was a problem hiding this comment.
This looks wrong to me. We should keep the iterator and range overload tests equivalent. Same for a few cases below.
There was a problem hiding this comment.
I made some efforts trying to unify those two sets of tests, but since contiguous_iterator is only designed to hold pointers (as is suggested here: test_iterators.h#L344), I couldn't find an easy way to do that. I ended up deleting that static assertion and made some changes to contiguous_iterator.
Those changes did not break any ranges tests, but since we use this class quite extensively, I would like your opinion on whether those changes make sense to the rest of the test suite.
There was a problem hiding this comment.
I don't think we should remove that static assertion. I'm also not sure what the problem was you were trying to solve with that change. I you need a range you can just do std::ranges::subrange(Iter(array.data()), Sent(array.data() + array.size())).
There was a problem hiding this comment.
For last in verify_heap, we get contiguous_iterator<int*> in some cases and std::array::iterator in some other cases, and we later on want to compare it with heapified.end(). Since there isn't a strictly conforming way to cast std::array::iterator to pointers (correct me if I'm wrong), the two solutions I was to either allow contiguous_iterator to contain std::array::iterator, or to check equality in different ways, as illustrated in e69008d.
In hindsight, the naming in e69008d could be misleading. If the general idea of it looks good to you, I can refine it as below:
template <std::size_t N, class T>
constexpr void verify_last(const std::array<T, N>& heapified, T *last) {
assert(last == heapified.data() + heapified.size());
}
template <std::size_t N, class T>
constexpr void verify_last(const std::array<T, N>& heapified, const typename std::array<T, N>::iterator& last) {
assert(last == heapified.end());
}
template <std::size_t N, class T, class Iter>
constexpr void verify_heap(const std::array<T, N>& heapified, Iter last, const std::array<T, N>& expected) {
assert(heapified == expected);
verify_last(heapified, base(last));
assert(std::is_heap(heapified.begin(), heapified.end()));
}There was a problem hiding this comment.
You can use std::to_address, but I think we should instead simply remove the uses of array::iterator, since we care about the algorithm and not the container.
There was a problem hiding this comment.
I agree that we should use pointers as much as possible for these tests. In practice, this might require a bigger rewriting of the tests. For now, I have updated verify_heap so that we are eventually checking equality of pointers.
There was a problem hiding this comment.
@philnik777 Do the new changes look good to you?
|
Thanks for the patch! I second @philnik777 here, this needs no discussion, this is a pure improvement and we already strive to do that but it's just hard to spot with a single implementation. I'll let @philnik777 review this in detail and merge once satisfied. |
…r std::array are pointers
wdunicornpro
force-pushed
the
libcxx/test/fix_std_array_iterator
branch
2 times, most recently
from
06d99ab to
5eb391e
Compare
Co-authored-by: philnik777 <[email protected]>
wdunicornpro
force-pushed
the
libcxx/test/fix_std_array_iterator
branch
from
5eb391e to
4724f60
Compare
philnik777
approved these changes
Nov 12, 2023
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…lvm#70729) This is to modify a list of libcxx tests written under the assumption that iterators for std::array, std::string_view, and std::string are pointers. The motivation for this PR is to make the tests more universal and potentially being used to test other C++ standard library implementations, for example [microsoft/STL](https://github.com/microsoft/STL). I can confirm that this patch makes a number of tests compatible with microsoft STL: `Failed : 204 (2.12%)` -> `Failed : 136 (1.42%)` , and does not break any tests on `libcxx`. This is not a complete list of such incompatibilities, but I am hoping this will start a discussion about whether we are open to accepting such changes.
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.
This is to modify a list of libcxx tests written under the assumption that iterators for std::array, std::string_view, and std::string are pointers. The motivation for this PR is to make the tests more universal and potentially being used to test other C++ standard library implementations, for example microsoft/STL.
I can confirm that this patch makes a number of tests compatible with microsoft STL:
Failed : 204 (2.12%)->Failed : 136 (1.42%), and does not break any tests on
libcxx.This is not a complete list of such incompatibilities, but I am hoping this will start a discussion about whether we are open to accepting such changes.