Skip to content

[libcxx][test] std::array::iterator are not pointers by C++ standard #70729

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.

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

Merged
merged 4 commits into from
Nov 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,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::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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down Expand Up @@ -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; }));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.end());
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.end());
assert(std::all_of(a.begin(), a.end(), [](auto& s) { return s == "long long string so no SSO"; }));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down Expand Up @@ -146,15 +146,15 @@ 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}));
assert(a.in1 == r.begin() + 3);
}
{
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong to me. We should keep the iterator and range overload tests equivalent. Same for a few cases below.

Copy link
Contributor Author

@wdunicornpro wdunicornpro Oct 31, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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())).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philnik777 Do the new changes look good to you?

assert(base(last) == heapified.data() + heapified.size());
assert(std::to_address(base(last)) == heapified.data() + heapified.size());
assert(std::is_heap(heapified.begin(), heapified.end()));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ 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(base(last) == heapified.data() + heapified.size());
assert(std::to_address(base(last)) == heapified.data() + heapified.size());
assert(std::is_heap(heapified.begin(), heapified.end() - 1));
assert(*std::max_element(heapified.begin(), heapified.end()) == heapified.back());
}
Expand Down Expand Up @@ -130,15 +130,15 @@ constexpr bool test() {
auto in = input;
auto last = std::ranges::pop_heap(in.begin(), in.end(), comp);
assert(in == expected);
assert(last == in.data() + in.size());
assert(last == in.end());
assert(std::is_heap(in.begin(), in.end() - 1, comp));
}

{
auto in = input;
auto last = std::ranges::pop_heap(in, comp);
assert(in == expected);
assert(last == in.data() + in.size());
assert(last == in.end());
assert(std::is_heap(in.begin(), in.end() - 1, comp));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ static_assert(!HasPushHeapR<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(base(last) == heapified.data() + heapified.size());
assert(std::to_address(base(last)) == heapified.data() + heapified.size());
assert(std::is_heap(heapified.begin(), heapified.end()));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ static_assert(!HasSortHeapR<UncheckedRange<const int*>>); // Doesn't satisfy `so
template <std::size_t N, class T, class Iter>
constexpr void verify_sorted(const std::array<T, N>& sorted, Iter last, std::array<T, N> expected) {
assert(sorted == expected);
assert(base(last) == sorted.data() + sorted.size());
assert(std::to_address(base(last)) == sorted.data() + sorted.size());
assert(std::is_sorted(sorted.begin(), sorted.end()));
}

Expand Down Expand Up @@ -131,14 +131,14 @@ constexpr bool test() {
auto in = input;
auto last = std::ranges::sort_heap(in.begin(), in.end(), comp);
assert(in == expected);
assert(last == in.data() + in.size());
assert(last == in.end());
}

{
auto in = input;
auto last = std::ranges::sort_heap(in, comp);
assert(in == expected);
assert(last == in.data() + in.size());
assert(last == in.end());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
assert(std::ranges::equal(out, std::array<TracedCopy, 6>{1, 3, 3, 5, 8, 8}));

assert(std::ranges::all_of(out, &TracedCopy::copiedOnce));
Expand All @@ -277,7 +277,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
assert(std::ranges::equal(out, std::array<TracedCopy, 6>{1, 3, 3, 5, 8, 8}));

assert(std::ranges::all_of(out, &TracedCopy::copiedOnce));
Expand Down Expand Up @@ -369,7 +369,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
}

// range overload
Expand All @@ -382,7 +382,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
}

// member pointer Comparator iterator overload
Expand All @@ -395,7 +395,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
}

// member pointer Comparator range overload
Expand All @@ -408,7 +408,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
}
}

Expand All @@ -431,7 +431,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
}

// range overload
Expand All @@ -444,7 +444,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
}

// member pointer Projection iterator overload
Expand All @@ -457,7 +457,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
}

// member pointer Projection range overload
Expand All @@ -470,7 +470,7 @@ constexpr bool test() {

assert(result.in1 == r1.end());
assert(result.in2 == r2.end());
assert(result.out == out.end());
assert(result.out == out.data() + out.size());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ template <std::size_t N, class T, class Iter>
constexpr void verify_nth(const std::array<T, N>& partially_sorted, std::size_t nth_index, Iter last, T expected_nth) {
// Note that the exact output of `nth_element` is unspecified and may vary between implementations.

assert(base(last) == partially_sorted.end());
assert(base(last) == partially_sorted.data() + partially_sorted.size());

auto b = partially_sorted.begin();
auto nth = b + nth_index;
Expand Down
Loading