Page MenuHomePhabricator

[libc++] Refactor the ranges::prev and ranges::next tests
ClosedPublic

Authored by ldionne on May 27 2021, 11:18 AM.

Details

Summary

This started as an attempt to fix a GCC 11 warning of misplaced parentheses.
I then noticed that trying to fix the parentheses warning actually triggered
errors in the tests, showing that we were incorrectly assuming that the
implementation of ranges::advance was using operator+= or operator-=.

This commit fixes that issue and makes the tests easier to follow by
localizing the assertions it makes. It also doesn't try to pretend that
we can use ranges::next with a straight output_iterator, which isn't
the case in reality.

Diff Detail

Event Timeline

ldionne requested review of this revision.May 27 2021, 11:18 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 11:18 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Partly reviewed, but had to step away in the middle.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/constraints.verify.cpp
0

It would be even nicer to make this a real SFINAE test and enable it for gcc-10/gcc-11.

zoecarver accepted this revision as: zoecarver.May 27 2021, 2:51 PM
zoecarver added a subscriber: zoecarver.

LGTM, thanks for the cleanup.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/constraints.verify.cpp
0

I agree. I think wherever possible we should move away from .verify tests. This could just be is_invocable_v.

The only place we should really use the .verify tests is when we're checking a static_assert.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp
81

Do we want to add other checks for output_iterator? Why did you remove this check?

ldionne updated this revision to Diff 348510.May 28 2021, 6:32 AM
ldionne marked 3 inline comments as done.

Apply review comments.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp
81

Those overloads of std::ranges::next can't be used with an output_iterator out of the box, since output_iterator is missing a comparison operator (we need to compare against the sentinel). I'm still testing against output_iterators for the next(iter) and next(iter, n) overloads, though.

Quuxplusone added inline comments.May 28 2021, 6:33 AM
libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp
49–64

Ooh, this test is super subtle. I recommend rewriting it as

template <class It>
constexpr void check_assignable_sentinel_case() {
  int range[] = {1, 2, 3, 4};
  It first = It(stride_counting_iterator<int*>(range + 1));
  It last = It(stride_counting_iterator<int*>(range + 4));
  It result = std::ranges::next(std::move(first), std::move(last));
  assert(base(result).base() == range + 4);  // i.e. last
  assert(result.stride_count() == 0);  // because we got here by copying last's value, not by incrementing
}
[...]
check_assignable_sentinel_case<cpp17_input_iterator<stride_counting_iterator<int*>>>();
check_assignable_sentinel_case<cpp20_input_iterator<stride_counting_iterator<int*>>>();
check_assignable_sentinel_case<output_iterator<stride_counting_iterator<int*>>>();
check_assignable_sentinel_case<forward_iterator<stride_counting_iterator<int*>>>();

etc. etc. In particular, I'm willing to believe sans tests that cpp17_input_iterator<stride_counting_iterator<int*>> is a cpp17_input_iterator, but I'm not willing to believe the same of stride_counting_iterator<cpp17_input_iterator<int*>>.

81

+1; I don't understand the bit of @ldionne's summary about not being able to use next on straight output iterators. cppreference thinks you can increment an output iterator just fine. So why was this line removed? And why weren't similar lines in other tests removed? (Grep the right side for output_iterator — we're still using it in plenty of places, e.g. line 32 of range.iter.ops.next/iterator.pass.cpp.) So I don't get it.

112–113

Please replace cpp17_input_iterator<range_t::const_iterator> with cpp17_input_iterator<const int*> and so on, throughout this file and anywhere else you see this pattern happening.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator.pass.cpp
19–32

FWIW, I'd prefer to see this simplified down to

template<class It>
constexpr bool test() {
  int range[] = {0, 1, 2, 3, 4, 5};
  assert(std::ranges::prev(It(range+1)) == It(range));
  assert(std::ranges::prev(It(range+5)) == It(range+4));
  assert(std::ranges::prev(It(range+6)) == It(range+5));
  return true;
}

int main(int, char**) {
  static_assert(test<bidirectional_iterator<int*>>());
  static_assert(test<random_access_iterator<int*>>());
  static_assert(test<contiguous_iterator<int*>>());
  static_assert(test<int*>());
  test<bidirectional_iterator<int*>>();
  test<random_access_iterator<int*>>();
  test<contiguous_iterator<int*>>();
  test<int*>();
}

This shortens the line lengths, and makes sure we hit the "one-past-the-end" case in constexpr for all kinds of iterators including raw pointers.

Quuxplusone added inline comments.May 28 2021, 6:38 AM
libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp
81

Our comments crossed in the ether (and also my understanding of this test is evolving). check_assignable_case is super subtle: it does not depend on comparing iterators at all. It simply says, "Ah, output_iterator is assignable from output_iterator, so std::ranges::next(it1, it2) is simply the result of it1 = it2, full stop." There is no comparison involved.
See the sample implementation of advance here: https://en.cppreference.com/w/cpp/iterator/ranges/advance , the part that mentions std::assignable_from. (The sample implementation of next is done in terms of advance.)

ldionne updated this revision to Diff 351560.Jun 11 2021, 2:02 PM
ldionne marked 5 inline comments as done.

Address review comments and tweak tests for advance too.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp
81

This is the signature of next(it, sent):

template<input_­or_­output_­iterator I, sentinel_­for<I> S>
constexpr I ranges::next(I x, S bound);

In particular, it requires sentinel_­for<I> S, which is not satisfied here because output_iterator<T> is not a sentinel for output_iterator<T> (it's missing the comparison). So it's simply invalid to try and call next(it, sent) when both it and sent are an output iterator archetype - we'd have to add something to the archetype to make it model sentinel_for.

It is true the implementation doesn't technically require comparing against the sentinel, but the signature still requires it.

Quuxplusone added inline comments.Jun 11 2021, 3:14 PM
libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator.pass.cpp
22–28

Simply:

int range[4] = {};
assert(base(std::ranges::next(It(range))) == range+1);
assert(base(std::ranges::next(It(range+3))) == range+4);
43–45

Bikeshed: Traditionally we put the runtime test first and the longer static_assert second.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp
52–53

Here and throughout this function, the autos are really confusing. Is this auto the same thing as I?
Is &*result the same thing as base(result)? (base being an overload set defined in "test_iterators.h".)

81

OK, I buy that. I think there should be a runtime test verifying that (although operator==/!= is required to exist) nothing except operator= assignment is ever actually called in this case.

120

Since you're changing this code anyway, FWIW I'd prefer
forward_iterator<int*>(range+k)
rather than
forward_iterator(&range[k])
throughout. (And I doubt that the variation of k0, 1, ... 7 — is pulling its weight. I'd leave them all as FooIter<int*>(range) for brevity, or else FooIter<int*>(range+10) for verification-that-these-pointers-are-never-dereferenced. And there's no reason range has to be so big; making it, say, size-4 instead of size-10 would be fine.)

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/constraints.compile.pass.cpp
23

I think you intended Args&&... args here. Otherwise the forward below should just be move (and it's certainly redundant either way, given that Args... here are always scalar types).

32–33

I'd do this by

using Jt = bidirectional_iterator<int*>;
static_assert(has_ranges_prev<Jt>);
static_assert(has_ranges_prev<Jt, std::ptrdiff_t>);
static_assert(has_ranges_prev<Jt, std::ptrdiff_t, Jt>);
libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator.pass.cpp
22–25
int range[4] = {};
assert(std::ranges::prev(It(range+1)) == It(range));
assert(std::ranges::prev(It(range+4)) == It(range+3));

It turns out I'm never happy. ;)

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count.pass.cpp
30

s/auto const/std::ptrdiff_t/ for sanity's sake.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp
31

Please declare one variable per line.
This smells like CTAD — is stride_counting_iterator a template? if so, could we write out its arguments?

libcxx/test/support/test_iterators.h
732–734

While you're here, you might remove the [[nodiscard]].

This revision was not accepted when it landed; it landed in state Needs Review.Jun 14 2021, 5:13 AM
This revision was automatically updated to reflect the committed changes.
ldionne marked 9 inline comments as done.Jun 14 2021, 5:43 AM
ldionne added inline comments.
libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator.pass.cpp
22–28

I wanted the test to be agnostic to the fact that It is an iterator archetype or not.

43–45

Indeed, will fix throughout.

libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp
52–53

As I said elsewhere, &* is to avoid relying on the fact that it's an iterator archetype. Will fix auto.