* [PATCH] libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() @ 2020-01-21 23:23 Patrick Palka 2020-01-29 14:00 ` Jonathan Wakely 0 siblings, 1 reply; 5+ messages in thread From: Patrick Palka @ 2020-01-21 23:23 UTC (permalink / raw) To: gcc-patches; +Cc: libstdc++, jwakely, Patrick Palka It seems that in practice std::sentinel_for<I, I> is always true, and so the test_range container doesn't help us detect bugs in ranges code in which we wrongly assume that a sentinel can be manipulated like an iterator. Make the test_range container more strict by having end() unconditionally return a sentinel<I>. Is this OK to commit after bootstrap+regtesting succeeds on x86_64-pc-linux-gnu? libstdc++-v3/ChangeLog: * testsuite/util/testsuite_iterators.h (__gnu_test::test_range::end): Always return a sentinel<I>. --- libstdc++-v3/testsuite/util/testsuite_iterators.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/libstdc++-v3/testsuite/util/testsuite_iterators.h b/libstdc++-v3/testsuite/util/testsuite_iterators.h index eb15257bf6a..6667a3af93a 100644 --- a/libstdc++-v3/testsuite/util/testsuite_iterators.h +++ b/libstdc++-v3/testsuite/util/testsuite_iterators.h @@ -702,10 +702,7 @@ namespace __gnu_test auto end() & { using I = decltype(get_iterator(bounds.last)); - if constexpr (std::sentinel_for<I, I>) - return get_iterator(bounds.last); - else - return sentinel<I>{bounds.last}; + return sentinel<I>{bounds.last}; } typename Iter<T>::ContainerType bounds; -- 2.25.0.rc0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() 2020-01-21 23:23 [PATCH] libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() Patrick Palka @ 2020-01-29 14:00 ` Jonathan Wakely 2020-01-29 16:43 ` Patrick Palka 0 siblings, 1 reply; 5+ messages in thread From: Jonathan Wakely @ 2020-01-29 14:00 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, libstdc++, Patrick Palka [-- Attachment #1: Type: text/plain, Size: 2085 bytes --] On 21/01/20 17:26 -0500, Patrick Palka wrote: >It seems that in practice std::sentinel_for<I, I> is always true, and so the Doh, good catch. >test_range container doesn't help us detect bugs in ranges code in which we >wrongly assume that a sentinel can be manipulated like an iterator. Make the >test_range container more strict by having end() unconditionally return a >sentinel<I>. Yes, this seems useful. >Is this OK to commit after bootstrap+regtesting succeeds on x86_64-pc-linux-gnu? As you mentioned on IRC, this makes some tests fail. Some of them are bad tests and this change reveals that, e.g. std/ranges/access/end.cc should not assume that r.end()==r.end() is valid (because sentinels can't be compared with sentinels). In other cases, the test is intentionally relying on the fact the r.end() returns the same type as r.begin(), e.g. in std/ranges/access/rbegin.cc we want to test this case: — Otherwise, make_reverse_iterator(ranges::end(t)) if both ranges::begin(t) and ranges::end(t) are valid expressions of the same type I which models bidirectional_iterator (23.3.4.12). If the sentinel returned by ranges::end(r) is a different type, we can't use test_range to verify this part of the ranges::rbegin requirements. I think we do want your change, so this patch fixes the tests that inappropriately assume the sentinel is the same type as the iterator. When we are actually relying on that property for the test, I'm adding a new type derived from test_range which provides that guarantee and using that instead. There's one final change needed to make std/ranges/access/size.cc compile after your patch, which is to make the test_range::sentinel type satisfy std::sized_sentinel_for when the iterator satisfies std::random_access_iterator, i.e. support subtracting iterators and sentinels from each other. Tested powerpc64le-linux, committed to trunk. Please go ahead and commit your patch to test_range::end() after re-testing. Thanks, and congratulations on your first libstdc++ patch. [-- Attachment #2: patch.txt --] [-- Type: text/x-patch, Size: 9919 bytes --] commit 5cd2e126f5c5705fa1aa7fafef3d6b94a99593da Author: Jonathan Wakely <jwakely@redhat.com> Date: Wed Jan 29 13:36:15 2020 +0000 libstdc++: Make tests for std::ranges access functions more robust * testsuite/std/ranges/access/end.cc: Do not assume test_range::end() returns the same type as test_range::begin(). Add comments. * testsuite/std/ranges/access/rbegin.cc: Likewise. * testsuite/std/ranges/access/rend.cc: Likewise. * testsuite/std/ranges/range.cc: Do not assume the sentinel for test_range is the same as its iterator type. * testsuite/util/testsuite_iterators.h (test_range::sentinel): Add operator- overloads to satisfy sized_sentinel_for when the iterator satisfies random_access_iterator. diff --git a/libstdc++-v3/testsuite/std/ranges/access/end.cc b/libstdc++-v3/testsuite/std/ranges/access/end.cc index 2bb0e52cf6e..c3a1028dc14 100644 --- a/libstdc++-v3/testsuite/std/ranges/access/end.cc +++ b/libstdc++-v3/testsuite/std/ranges/access/end.cc @@ -29,6 +29,8 @@ test01() { int a[2] = {}; + // t + extent_v<T> if E is of array type T. + static_assert(same_as<decltype(std::ranges::end(a)), decltype(a + 2)>); static_assert(noexcept(std::ranges::end(a))); VERIFY( std::ranges::end(a) == (a + 2) ); @@ -44,13 +46,16 @@ test02() int a[] = { 0, 1 }; + // Otherwise, decay-copy(t.end()) if it is a valid expression + // and its type S models sentinel_for<decltype(ranges::begin(E))>. + test_range<int, random_access_iterator_wrapper> r(a); static_assert(same_as<decltype(std::ranges::end(r)), decltype(r.end())>); - VERIFY( std::ranges::end(r) == r.end() ); + VERIFY( std::ranges::end(r) == std::ranges::next(r.begin(), 2) ); test_range<int, input_iterator_wrapper> i(a); static_assert(same_as<decltype(std::ranges::end(i)), decltype(i.end())>); - VERIFY( std::ranges::end(i) == i.end() ); + VERIFY( std::ranges::end(i) == std::ranges::next(i.begin(), 2) ); test_range<int, output_iterator_wrapper> o(a); static_assert(same_as<decltype(std::ranges::end(o)), decltype(o.end())>); @@ -93,6 +98,9 @@ test03() R r; const R& c = r; + // Otherwise, decay-copy(end(t)) if it is a valid expression + // and its type S models sentinel_for<decltype(ranges::begin(E))>. + static_assert(same_as<decltype(std::ranges::end(r)), decltype(end(r))>); static_assert(!noexcept(std::ranges::end(r))); VERIFY( std::ranges::end(r) == end(r) ); diff --git a/libstdc++-v3/testsuite/std/ranges/access/rbegin.cc b/libstdc++-v3/testsuite/std/ranges/access/rbegin.cc index 03e73a9cb4a..e92e5bc69ac 100644 --- a/libstdc++-v3/testsuite/std/ranges/access/rbegin.cc +++ b/libstdc++-v3/testsuite/std/ranges/access/rbegin.cc @@ -38,6 +38,8 @@ void test01() { constexpr R1 r; + // decay-copy(t.rbegin()) if it is a valid expression + // and its type I models input_or_output_iterator. static_assert( std::ranges::rbegin(r) == &r.i ); static_assert( std::ranges::rbegin(std::move(r)) == &r.i ); } @@ -60,6 +62,8 @@ void test02() { constexpr R2 r; + // Otherwise, decay-copy(rbegin(t)) if it is a valid expression + // and its type I models input_or_output_iterator [...] static_assert( std::ranges::rbegin(r) == std::make_reverse_iterator(std::ranges::end(r)) ); static_assert( std::ranges::rbegin(std::move(r)) @@ -69,11 +73,29 @@ test02() void test03() { - using __gnu_test::test_range; - using __gnu_test::bidirectional_iterator_wrapper; + struct R3 + : __gnu_test::test_range<int, __gnu_test::bidirectional_iterator_wrapper> + { + R3(int (&a)[2]) : test_range(a) { } + + using test_range::begin; + + // Replace test_range::end() to return same type as begin() + // so ranges::rbegin will wrap it in a reverse_iterator . + auto end() & + { + using __gnu_test::bidirectional_iterator_wrapper; + return bidirectional_iterator_wrapper<int>(bounds.last, &bounds); + } + }; int a[2] = { }; - test_range<int, bidirectional_iterator_wrapper> r(a); + R3 r(a); + + // Otherwise, make_reverse_iterator(ranges::end(t)) if both ranges::begin(t) + // and ranges::end(t) are valid expressions of the same type I which models + // bidirectional_iterator. + VERIFY( std::ranges::rbegin(r) == std::make_reverse_iterator(std::ranges::end(r)) ); } diff --git a/libstdc++-v3/testsuite/std/ranges/access/rend.cc b/libstdc++-v3/testsuite/std/ranges/access/rend.cc index 42816d99359..f6909b8340c 100644 --- a/libstdc++-v3/testsuite/std/ranges/access/rend.cc +++ b/libstdc++-v3/testsuite/std/ranges/access/rend.cc @@ -40,11 +40,41 @@ void test01() { constexpr R1 r; + + // decay-copy(t.rend()) if it is a valid expression + // and its type S models sentinel_for<decltype(ranges::rbegin(E))> + static_assert( std::ranges::rend(r) == &r.i + 1 ); static_assert( std::ranges::rend(std::move(r)) == &r.i + 1 ); } struct R2 +{ + int i = 0; + + int* rbegin() noexcept { return &i + 1; } + long* rend() noexcept { return nullptr; } // not a sentinel for rbegin() + + friend long* rbegin(R2&) noexcept { return nullptr; } + friend int* rend(R2& r) { return &r.i; } +}; + +void +test02() +{ + R2 r; + + // Otherwise, decay-copy(rend(t)) if it is a valid expression + // and its type S models sentinel_for<decltype(ranges::rbegin(E))> + + auto i1 = std::ranges::rbegin(r); + auto i2 = rend(r); + static_assert( std::sentinel_for<decltype(i2), decltype(i1)> ); + VERIFY( std::ranges::rend(r) == &r.i ); + static_assert( !noexcept(std::ranges::rend(r)) ); +} + +struct R3 { int a[2] = { }; long l[2] = { }; @@ -52,53 +82,49 @@ struct R2 constexpr const int* begin() const { return a; } constexpr const int* end() const { return a + 2; } - friend constexpr const long* begin(const R2&& r) { return r.l; } - friend constexpr const long* end(const R2&& r) { return r.l + 2; } + friend constexpr const long* begin(const R3&& r) { return r.l; } + friend constexpr const long* end(const R3&& r) { return r.l + 2; } }; -// N.B. this is a lie, begin/end on an R2 rvalue will return a dangling pointer. -template<> constexpr bool std::ranges::enable_safe_range<R2> = true; +// N.B. this is a lie, begin/end on an R3 rvalue will return a dangling pointer. +template<> constexpr bool std::ranges::enable_safe_range<R3> = true; void -test02() +test03() { - constexpr R2 r; + constexpr R3 r; + + // Otherwise, make_reverse_iterator(ranges::begin(t)) if both + // ranges::begin(t) and ranges::end(t) are valid expressions + // of the same type I which models bidirectional_iterator. + static_assert( std::ranges::rend(r) == std::make_reverse_iterator(std::ranges::begin(r)) ); static_assert( std::ranges::rend(std::move(r)) == std::make_reverse_iterator(std::ranges::begin(std::move(r))) ); } -struct R3 -{ - int i = 0; - - int* rbegin() noexcept { return &i + 1; } - long* rend() noexcept { return nullptr; } // not a sentinel for rbegin() - - friend long* rbegin(R3&) noexcept { return nullptr; } - friend int* rend(R3& r) { return &r.i; } -}; - -void -test03() -{ - R3 r; - auto i1 = std::ranges::rbegin(r); - auto i2 = rend(r); - static_assert( std::sentinel_for<decltype(i2), decltype(i1)> ); - // VERIFY( std::ranges::rend(r) == r.i ); - // static_assert( !noexcept(std::ranges::rend(r)) ); -} - void test04() { - using __gnu_test::test_range; - using __gnu_test::bidirectional_iterator_wrapper; + struct R4 + : __gnu_test::test_range<int, __gnu_test::bidirectional_iterator_wrapper> + { + R4(int (&a)[2]) : test_range(a) { } + + using test_range::begin; + + // Replace test_range::end() to return same type as begin() + // so ranges::rend will wrap it in a reverse_iterator. + auto end() & + { + using __gnu_test::bidirectional_iterator_wrapper; + return bidirectional_iterator_wrapper<int>(bounds.last, &bounds); + } + }; int a[2] = { }; - test_range<int, bidirectional_iterator_wrapper> r(a); + R4 r(a); VERIFY( std::ranges::rend(r) == std::make_reverse_iterator(std::ranges::begin(r)) ); } @@ -108,4 +134,5 @@ main() test01(); test02(); test03(); + test04(); } diff --git a/libstdc++-v3/testsuite/std/ranges/range.cc b/libstdc++-v3/testsuite/std/ranges/range.cc index fabbcf90853..cf349de8735 100644 --- a/libstdc++-v3/testsuite/std/ranges/range.cc +++ b/libstdc++-v3/testsuite/std/ranges/range.cc @@ -66,7 +66,7 @@ static_assert( same_as<std::ranges::iterator_t<O>, decltype(std::declval<O&>().begin())> ); static_assert( same_as<std::ranges::sentinel_t<C>, - contiguous_iterator_wrapper<char>> ); + decltype(std::declval<C&>().end())> ); static_assert( same_as<std::ranges::sentinel_t<O>, decltype(std::declval<O&>().end())> ); diff --git a/libstdc++-v3/testsuite/util/testsuite_iterators.h b/libstdc++-v3/testsuite/util/testsuite_iterators.h index eb15257bf6a..1c7fbd001e0 100644 --- a/libstdc++-v3/testsuite/util/testsuite_iterators.h +++ b/libstdc++-v3/testsuite/util/testsuite_iterators.h @@ -675,8 +675,16 @@ namespace __gnu_test { T* end; - friend bool operator==(const sentinel& s, const I& i) + friend bool operator==(const sentinel& s, const I& i) noexcept { return s.end == i.ptr; } + + friend auto operator-(const sentinel& s, const I& i) noexcept + requires std::random_access_iterator<I> + { return s.end - i.ptr; } + + friend auto operator-(const I& i, const sentinel& s) noexcept + requires std::random_access_iterator<I> + { return i.ptr - s.end; } }; auto ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() 2020-01-29 14:00 ` Jonathan Wakely @ 2020-01-29 16:43 ` Patrick Palka 2020-01-29 17:35 ` Patrick Palka 0 siblings, 1 reply; 5+ messages in thread From: Patrick Palka @ 2020-01-29 16:43 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Patrick Palka, gcc-patches, libstdc++, Patrick Palka [-- Attachment #1: Type: text/plain, Size: 2367 bytes --] On Wed, 29 Jan 2020, Jonathan Wakely wrote: > On 21/01/20 17:26 -0500, Patrick Palka wrote: >> It seems that in practice std::sentinel_for<I, I> is always true, and so >> the > > Doh, good catch. > >> test_range container doesn't help us detect bugs in ranges code in which we >> wrongly assume that a sentinel can be manipulated like an iterator. Make >> the >> test_range container more strict by having end() unconditionally return a >> sentinel<I>. > > Yes, this seems useful. > >> Is this OK to commit after bootstrap+regtesting succeeds on >> x86_64-pc-linux-gnu? > > As you mentioned on IRC, this makes some tests fail. > > Some of them are bad tests and this change reveals that, e.g. > std/ranges/access/end.cc should not assume that r.end()==r.end() is > valid (because sentinels can't be compared with sentinels). > > In other cases, the test is intentionally relying on the fact the > r.end() returns the same type as r.begin(), e.g. in > std/ranges/access/rbegin.cc we want to test this case: > > — Otherwise, make_reverse_iterator(ranges::end(t)) if both > ranges::begin(t) and ranges::end(t) are valid expressions of the same > type I which models bidirectional_iterator (23.3.4.12). > > If the sentinel returned by ranges::end(r) is a different type, we > can't use test_range to verify this part of the ranges::rbegin > requirements. > > I think we do want your change, so this patch fixes the tests that > inappropriately assume the sentinel is the same type as the iterator. > When we are actually relying on that property for the test, I'm adding > a new type derived from test_range which provides that guarantee and > using that instead. > > There's one final change needed to make std/ranges/access/size.cc > compile after your patch, which is to make the test_range::sentinel > type satisfy std::sized_sentinel_for when the iterator satisfies > std::random_access_iterator, i.e. support subtracting iterators and > sentinels from each other. > > Tested powerpc64le-linux, committed to trunk. Thanks for the review and the testsuite fixes. > > Please go ahead and commit your patch to test_range::end() after > re-testing. Thanks, and congratulations on your first libstdc++ > patch. Thanks! :) I am re-testing now, and will commit the patch after that's done. > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() 2020-01-29 16:43 ` Patrick Palka @ 2020-01-29 17:35 ` Patrick Palka 2020-01-31 11:20 ` Jonathan Wakely 0 siblings, 1 reply; 5+ messages in thread From: Patrick Palka @ 2020-01-29 17:35 UTC (permalink / raw) To: Patrick Palka; +Cc: Jonathan Wakely, gcc-patches, libstdc++, Patrick Palka [-- Attachment #1: Type: text/plain, Size: 17971 bytes --] On Wed, 29 Jan 2020, Patrick Palka wrote: > On Wed, 29 Jan 2020, Jonathan Wakely wrote: > >> On 21/01/20 17:26 -0500, Patrick Palka wrote: >>> It seems that in practice std::sentinel_for<I, I> is always true, and so >>> the >> >> Doh, good catch. >> >>> test_range container doesn't help us detect bugs in ranges code in which >>> we >>> wrongly assume that a sentinel can be manipulated like an iterator. Make >>> the >>> test_range container more strict by having end() unconditionally return a >>> sentinel<I>. >> >> Yes, this seems useful. >> >>> Is this OK to commit after bootstrap+regtesting succeeds on >>> x86_64-pc-linux-gnu? >> >> As you mentioned on IRC, this makes some tests fail. >> >> Some of them are bad tests and this change reveals that, e.g. >> std/ranges/access/end.cc should not assume that r.end()==r.end() is >> valid (because sentinels can't be compared with sentinels). >> >> In other cases, the test is intentionally relying on the fact the >> r.end() returns the same type as r.begin(), e.g. in >> std/ranges/access/rbegin.cc we want to test this case: >> >> — Otherwise, make_reverse_iterator(ranges::end(t)) if both >> ranges::begin(t) and ranges::end(t) are valid expressions of the same >> type I which models bidirectional_iterator (23.3.4.12). >> >> If the sentinel returned by ranges::end(r) is a different type, we >> can't use test_range to verify this part of the ranges::rbegin >> requirements. >> >> I think we do want your change, so this patch fixes the tests that >> inappropriately assume the sentinel is the same type as the iterator. >> When we are actually relying on that property for the test, I'm adding >> a new type derived from test_range which provides that guarantee and >> using that instead. >> >> There's one final change needed to make std/ranges/access/size.cc >> compile after your patch, which is to make the test_range::sentinel >> type satisfy std::sized_sentinel_for when the iterator satisfies >> std::random_access_iterator, i.e. support subtracting iterators and >> sentinels from each other. >> >> Tested powerpc64le-linux, committed to trunk. > > Thanks for the review and the testsuite fixes. > >> >> Please go ahead and commit your patch to test_range::end() after >> re-testing. Thanks, and congratulations on your first libstdc++ >> patch. > > Thanks! :) I am re-testing now, and will commit the patch after that's > done. It looks like there are other testsuite failures that need to get addressed, in particular in the tests range_operations/{prev,distance,next}.cc. In prev.cc and next.cc we are passing a sentinel as the first argument to ranges::next and ranges::prev, which expect an iterator as their first argument. In distance.cc we're passing a sentinel as the first argument to ranges::distance, which also expects an iterator as its first argument. Here's an updated patch that adjusts these tests in the straightforward way. In test04() in next.cc I had to reset the bounds on the container after doing ranges::next(begin, end) since we're manipulating an input_iterator_wrapper. Is this OK to commit? -- >8 -- Subject: [PATCH] libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() It seems that in practice std::sentinel_for<I, I> is always true, and so the test_range container doesn't help us detect bugs in ranges code in which we wrongly assume that a sentinel can be manipulated like an iterator. Make the test_range range more strict by having end() unconditionally return a sentinel<I>, and adjust some tests accordingly. libstdc++-v3/ChangeLog: * testsuite/24_iterators/range_operations/distance.cc: Do not assume test_range::end() returns the same type as test_range::begin(). * testsuite/24_iterators/range_operations/next.cc: Likewise. * testsuite/24_iterators/range_operations/prev.cc: Likewise. * testsuite/util/testsuite_iterators.h (__gnu_test::test_range::end): Always return a sentinel<I>. --- .../24_iterators/range_operations/distance.cc | 30 ++++++---- .../24_iterators/range_operations/next.cc | 57 ++++++++++--------- .../24_iterators/range_operations/prev.cc | 50 ++++++++-------- .../testsuite/util/testsuite_iterators.h | 5 +- 4 files changed, 78 insertions(+), 64 deletions(-) diff --git a/libstdc++-v3/testsuite/24_iterators/range_operations/distance.cc b/libstdc++-v3/testsuite/24_iterators/range_operations/distance.cc index 754c0cc200b..cf251b04ec5 100644 --- a/libstdc++-v3/testsuite/24_iterators/range_operations/distance.cc +++ b/libstdc++-v3/testsuite/24_iterators/range_operations/distance.cc @@ -39,13 +39,17 @@ test01() test_range<int, random_access_iterator_wrapper> c(a); VERIFY( std::ranges::distance(c) == 10 ); - auto b = c.begin(), e = c.end(); + auto b = c.begin(); + auto e = c.end(); + auto ei = std::ranges::next(b, e); VERIFY( std::ranges::distance(b, e) == 10 ); - VERIFY( std::ranges::distance(e, b) == -10 ); + VERIFY( std::ranges::distance(ei, b) == -10 ); - const auto cb = b, ce = e; + const auto cb = b; + const auto ce = e; + const auto cei = ei; VERIFY( std::ranges::distance(cb, ce) == 10 ); - VERIFY( std::ranges::distance(ce, cb) == -10 ); + VERIFY( std::ranges::distance(cei, cb) == -10 ); test_sized_range<int, random_access_iterator_wrapper> c2(a); VERIFY( std::ranges::distance(c2) == 10 ); @@ -60,10 +64,12 @@ test02() test_range<int, bidirectional_iterator_wrapper> c(a); VERIFY( std::ranges::distance(c) == 2 ); - auto b = c.begin(), e = c.end(); + auto b = c.begin(); + auto e = c.end(); VERIFY( std::ranges::distance(b, e) == 2 ); - const auto cb = b, ce = e; + const auto cb = b; + const auto ce = e; VERIFY( std::ranges::distance(cb, ce) == 2 ); test_sized_range<int, bidirectional_iterator_wrapper> c2(a); @@ -77,10 +83,12 @@ test03() test_range<int, forward_iterator_wrapper> c(a); VERIFY( std::ranges::distance(c) == 3 ); - auto b = c.begin(), e = c.end(); + auto b = c.begin(); + auto e = c.end(); VERIFY( std::ranges::distance(b, e) == 3 ); - const auto cb = b, ce = e; + const auto cb = b; + const auto ce = e; VERIFY( std::ranges::distance(cb, ce) == 3 ); test_sized_range<int, forward_iterator_wrapper> c2(a); @@ -99,11 +107,13 @@ test04() VERIFY( std::ranges::distance(c) == 0 ); c = test_range<int, input_iterator_wrapper>(a); - auto b = c.begin(), e = c.end(); + auto b = c.begin(); + auto e = c.end(); VERIFY( std::ranges::distance(b, e) == 4 ); test_range<int, input_iterator_wrapper> c2(a); - const auto cb = c2.begin(), ce = c2.end(); + const auto cb = c2.begin(); + const auto ce = c2.end(); VERIFY( std::ranges::distance(cb, ce) == 4 ); test_sized_range<int, input_iterator_wrapper> c3(a); diff --git a/libstdc++-v3/testsuite/24_iterators/range_operations/next.cc b/libstdc++-v3/testsuite/24_iterators/range_operations/next.cc index af4ae7eacdb..d3fe7b47150 100644 --- a/libstdc++-v3/testsuite/24_iterators/range_operations/next.cc +++ b/libstdc++-v3/testsuite/24_iterators/range_operations/next.cc @@ -36,27 +36,28 @@ test01() test_range<int, random_access_iterator_wrapper> r(a); auto begin = r.begin(); auto end = r.end(); + auto endi = std::ranges::next(begin, end); VERIFY( *std::ranges::next(begin) == 1 ); VERIFY( std::ranges::next(begin, 0) == begin ); VERIFY( *std::ranges::next(begin, 1) == 1 ); VERIFY( *std::ranges::next(begin, 3) == 3 ); - VERIFY( *std::ranges::next(end, -4) == 6 ); + VERIFY( *std::ranges::next(endi, -4) == 6 ); VERIFY( std::ranges::next(begin, begin) == begin ); VERIFY( std::ranges::next(begin, end) == end ); - VERIFY( std::ranges::next(end, end) == end ); - VERIFY( std::ranges::next(end, begin) == begin ); + VERIFY( std::ranges::next(endi, end) == end ); + VERIFY( std::ranges::next(endi, begin) == begin ); VERIFY( std::ranges::next(begin, 0, begin) == begin ); VERIFY( std::ranges::next(begin, 5, begin) == begin ); VERIFY( std::ranges::next(begin, -5, begin) == begin ); VERIFY( std::ranges::next(begin, 0, end) == begin ); VERIFY( *std::ranges::next(begin, 5, end) == 5 ); VERIFY( std::ranges::next(begin, 55, end) == end ); - VERIFY( std::ranges::next(end, 0, end) == end ); - VERIFY( std::ranges::next(end, -5, end) == end ); - VERIFY( std::ranges::next(end, -55, end) == end ); - VERIFY( std::ranges::next(end, 0, begin) == end ); - VERIFY( *std::ranges::next(end, -5, begin) == 5 ); - VERIFY( std::ranges::next(end, -55, begin) == begin ); + VERIFY( std::ranges::next(endi, 0, end) == end ); + VERIFY( std::ranges::next(endi, -5, end) == end ); + VERIFY( std::ranges::next(endi, -55, end) == end ); + VERIFY( std::ranges::next(endi, 0, begin) == end ); + VERIFY( *std::ranges::next(endi, -5, begin) == 5 ); + VERIFY( std::ranges::next(endi, -55, begin) == begin ); } void @@ -66,27 +67,28 @@ test02() test_range<int, bidirectional_iterator_wrapper> r(a); auto begin = r.begin(); auto end = r.end(); + auto endi = std::ranges::next(begin, end); VERIFY( *std::ranges::next(begin) == 1 ); VERIFY( std::ranges::next(begin, 0) == begin ); VERIFY( *std::ranges::next(begin, 1) == 1 ); VERIFY( *std::ranges::next(begin, 3) == 3 ); - VERIFY( *std::ranges::next(end, -4) == 6 ); + VERIFY( *std::ranges::next(endi, -4) == 6 ); VERIFY( std::ranges::next(begin, begin) == begin ); VERIFY( std::ranges::next(begin, end) == end ); - VERIFY( std::ranges::next(end, end) == end ); - VERIFY( std::ranges::next(end, begin) == begin ); + VERIFY( std::ranges::next(endi, end) == end ); + VERIFY( std::ranges::next(endi, begin) == begin ); VERIFY( std::ranges::next(begin, 0, begin) == begin ); VERIFY( std::ranges::next(begin, 5, begin) == begin ); VERIFY( std::ranges::next(begin, -5, begin) == begin ); VERIFY( std::ranges::next(begin, 0, end) == begin ); VERIFY( *std::ranges::next(begin, 5, end) == 5 ); VERIFY( std::ranges::next(begin, 55, end) == end ); - VERIFY( std::ranges::next(end, 0, end) == end ); - VERIFY( std::ranges::next(end, -5, end) == end ); - VERIFY( std::ranges::next(end, -55, end) == end ); - VERIFY( std::ranges::next(end, 0, begin) == end ); - VERIFY( *std::ranges::next(end, -5, begin) == 5 ); - VERIFY( std::ranges::next(end, -55, begin) == begin ); + VERIFY( std::ranges::next(endi, 0, end) == end ); + VERIFY( std::ranges::next(endi, -5, end) == end ); + VERIFY( std::ranges::next(endi, -55, end) == end ); + VERIFY( std::ranges::next(endi, 0, begin) == end ); + VERIFY( *std::ranges::next(endi, -5, begin) == 5 ); + VERIFY( std::ranges::next(endi, -55, begin) == begin ); } void @@ -96,23 +98,24 @@ test03() test_range<int, forward_iterator_wrapper> r(a); auto begin = r.begin(); auto end = r.end(); + auto endi = std::ranges::next(begin, end); VERIFY( *std::ranges::next(begin) == 1 ); VERIFY( std::ranges::next(begin, 0) == begin ); VERIFY( *std::ranges::next(begin, 1) == 1 ); VERIFY( *std::ranges::next(begin, 3) == 3 ); VERIFY( std::ranges::next(begin, begin) == begin ); VERIFY( std::ranges::next(begin, end) == end ); - VERIFY( std::ranges::next(end, end) == end ); + VERIFY( std::ranges::next(endi, end) == end ); VERIFY( std::ranges::next(begin, 0, begin) == begin ); VERIFY( std::ranges::next(begin, 5, begin) == begin ); VERIFY( std::ranges::next(begin, -5, begin) == begin ); VERIFY( std::ranges::next(begin, 0, end) == begin ); VERIFY( *std::ranges::next(begin, 5, end) == 5 ); VERIFY( std::ranges::next(begin, 55, end) == end ); - VERIFY( std::ranges::next(end, 0, end) == end ); - VERIFY( std::ranges::next(end, 5, end) == end ); - VERIFY( std::ranges::next(end, 55, end) == end ); - VERIFY( std::ranges::next(end, 0, begin) == end ); + VERIFY( std::ranges::next(endi, 0, end) == end ); + VERIFY( std::ranges::next(endi, 5, end) == end ); + VERIFY( std::ranges::next(endi, 55, end) == end ); + VERIFY( std::ranges::next(endi, 0, begin) == end ); } void @@ -141,6 +144,8 @@ test04() test_range<int, input_iterator_wrapper> r2(a); begin = r2.begin(); end = r2.end(); + auto endi = std::ranges::next(begin, end); + r2.bounds.first = a; iter = std::ranges::next(begin, 0, begin); VERIFY( *iter == 0 ); iter = std::ranges::next(begin, 5, begin); @@ -149,15 +154,15 @@ test04() VERIFY( *iter == 0 ); iter = std::ranges::next(begin, 0, end); VERIFY( *iter == 0 ); - iter = std::ranges::next(end, 0, begin); + iter = std::ranges::next(endi, 0, begin); VERIFY( iter == end ); iter = std::ranges::next(begin, 5, end); // invalidates begin VERIFY( *iter == 5 ); iter = std::ranges::next(iter, 55, end); VERIFY( iter == end ); - iter = std::ranges::next(end, 0, end); + iter = std::ranges::next(endi, 0, end); VERIFY( iter == end ); - iter = std::ranges::next(end, 5, end); + iter = std::ranges::next(endi, 5, end); VERIFY( iter == end ); } diff --git a/libstdc++-v3/testsuite/24_iterators/range_operations/prev.cc b/libstdc++-v3/testsuite/24_iterators/range_operations/prev.cc index ebf7c3f1323..70064b1b810 100644 --- a/libstdc++-v3/testsuite/24_iterators/range_operations/prev.cc +++ b/libstdc++-v3/testsuite/24_iterators/range_operations/prev.cc @@ -36,23 +36,24 @@ test01() test_range<int, random_access_iterator_wrapper> r(a); auto begin = r.begin(); auto end = r.end(); - VERIFY( *std::ranges::prev(end) == 9 ); + auto endi = std::ranges::next(begin, end); + VERIFY( *std::ranges::prev(endi) == 9 ); VERIFY( std::ranges::prev(begin, 0) == begin ); - VERIFY( *std::ranges::prev(end, 1) == 9 ); - VERIFY( *std::ranges::prev(end, 3) == 7 ); + VERIFY( *std::ranges::prev(endi, 1) == 9 ); + VERIFY( *std::ranges::prev(endi, 3) == 7 ); VERIFY( *std::ranges::prev(begin, -4) == 4 ); VERIFY( std::ranges::prev(begin, 0, begin) == begin ); VERIFY( std::ranges::prev(begin, 5, begin) == begin ); VERIFY( std::ranges::prev(begin, -5, begin) == begin ); - VERIFY( std::ranges::prev(begin, 0, end) == begin ); - VERIFY( *std::ranges::prev(end, 5, begin) == 5 ); - VERIFY( std::ranges::prev(end, 55, begin) == begin ); - VERIFY( std::ranges::prev(end, 0, end) == end ); - VERIFY( std::ranges::prev(end, -5, end) == end ); - VERIFY( std::ranges::prev(end, -55, end) == end ); - VERIFY( std::ranges::prev(end, 0, begin) == end ); - VERIFY( *std::ranges::prev(begin, -5, end) == 5 ); - VERIFY( std::ranges::prev(begin, -55, end) == end ); + VERIFY( std::ranges::prev(begin, 0, endi) == begin ); + VERIFY( *std::ranges::prev(endi, 5, begin) == 5 ); + VERIFY( std::ranges::prev(endi, 55, begin) == begin ); + VERIFY( std::ranges::prev(endi, 0, endi) == end ); + VERIFY( std::ranges::prev(endi, -5, endi) == end ); + VERIFY( std::ranges::prev(endi, -55, endi) == end ); + VERIFY( std::ranges::prev(endi, 0, begin) == end ); + VERIFY( *std::ranges::prev(begin, -5, endi) == 5 ); + VERIFY( std::ranges::prev(begin, -55, endi) == end ); } void @@ -62,23 +63,24 @@ test02() test_range<int, bidirectional_iterator_wrapper> r(a); auto begin = r.begin(); auto end = r.end(); - VERIFY( *std::ranges::prev(end) == 9 ); + auto endi = std::ranges::next(begin, end); + VERIFY( *std::ranges::prev(endi) == 9 ); VERIFY( std::ranges::prev(begin, 0) == begin ); - VERIFY( *std::ranges::prev(end, 1) == 9 ); - VERIFY( *std::ranges::prev(end, 3) == 7 ); + VERIFY( *std::ranges::prev(endi, 1) == 9 ); + VERIFY( *std::ranges::prev(endi, 3) == 7 ); VERIFY( *std::ranges::prev(begin, -4) == 4 ); VERIFY( std::ranges::prev(begin, 0, begin) == begin ); VERIFY( std::ranges::prev(begin, 5, begin) == begin ); VERIFY( std::ranges::prev(begin, -5, begin) == begin ); - VERIFY( std::ranges::prev(begin, 0, end) == begin ); - VERIFY( *std::ranges::prev(end, 5, begin) == 5 ); - VERIFY( std::ranges::prev(end, 55, begin) == begin ); - VERIFY( std::ranges::prev(end, 0, end) == end ); - VERIFY( std::ranges::prev(end, -5, end) == end ); - VERIFY( std::ranges::prev(end, -55, end) == end ); - VERIFY( std::ranges::prev(end, 0, begin) == end ); - VERIFY( *std::ranges::prev(begin, -5, end) == 5 ); - VERIFY( std::ranges::prev(begin, -55, end) == end ); + VERIFY( std::ranges::prev(begin, 0, endi) == begin ); + VERIFY( *std::ranges::prev(endi, 5, begin) == 5 ); + VERIFY( std::ranges::prev(endi, 55, begin) == begin ); + VERIFY( std::ranges::prev(endi, 0, endi) == end ); + VERIFY( std::ranges::prev(endi, -5, endi) == end ); + VERIFY( std::ranges::prev(endi, -55, endi) == end ); + VERIFY( std::ranges::prev(endi, 0, begin) == end ); + VERIFY( *std::ranges::prev(begin, -5, endi) == 5 ); + VERIFY( std::ranges::prev(begin, -55, endi) == end ); } template<typename T> diff --git a/libstdc++-v3/testsuite/util/testsuite_iterators.h b/libstdc++-v3/testsuite/util/testsuite_iterators.h index 1c7fbd001e0..6887d806a31 100644 --- a/libstdc++-v3/testsuite/util/testsuite_iterators.h +++ b/libstdc++-v3/testsuite/util/testsuite_iterators.h @@ -710,10 +710,7 @@ namespace __gnu_test auto end() & { using I = decltype(get_iterator(bounds.last)); - if constexpr (std::sentinel_for<I, I>) - return get_iterator(bounds.last); - else - return sentinel<I>{bounds.last}; + return sentinel<I>{bounds.last}; } typename Iter<T>::ContainerType bounds; -- 2.25.0.24.gbc7a3d4dc0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() 2020-01-29 17:35 ` Patrick Palka @ 2020-01-31 11:20 ` Jonathan Wakely 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Wakely @ 2020-01-31 11:20 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, libstdc++, Patrick Palka On 29/01/20 11:24 -0500, Patrick Palka wrote: >On Wed, 29 Jan 2020, Patrick Palka wrote: > >>On Wed, 29 Jan 2020, Jonathan Wakely wrote: >> >>>On 21/01/20 17:26 -0500, Patrick Palka wrote: >>>>It seems that in practice std::sentinel_for<I, I> is always >>>>true, and so the >>> >>>Doh, good catch. >>> >>>>test_range container doesn't help us detect bugs in ranges code >>>>in which we >>>>wrongly assume that a sentinel can be manipulated like an >>>>iterator. Make the >>>>test_range container more strict by having end() unconditionally return a >>>>sentinel<I>. >>> >>>Yes, this seems useful. >>> >>>>Is this OK to commit after bootstrap+regtesting succeeds on >>>>x86_64-pc-linux-gnu? >>> >>>As you mentioned on IRC, this makes some tests fail. >>> >>>Some of them are bad tests and this change reveals that, e.g. >>>std/ranges/access/end.cc should not assume that r.end()==r.end() is >>>valid (because sentinels can't be compared with sentinels). >>> >>>In other cases, the test is intentionally relying on the fact the >>>r.end() returns the same type as r.begin(), e.g. in >>>std/ranges/access/rbegin.cc we want to test this case: >>> >>>— Otherwise, make_reverse_iterator(ranges::end(t)) if both >>> ranges::begin(t) and ranges::end(t) are valid expressions of the same >>> type I which models bidirectional_iterator (23.3.4.12). >>> >>>If the sentinel returned by ranges::end(r) is a different type, we >>>can't use test_range to verify this part of the ranges::rbegin >>>requirements. >>> >>>I think we do want your change, so this patch fixes the tests that >>>inappropriately assume the sentinel is the same type as the iterator. >>>When we are actually relying on that property for the test, I'm adding >>>a new type derived from test_range which provides that guarantee and >>>using that instead. >>> >>>There's one final change needed to make std/ranges/access/size.cc >>>compile after your patch, which is to make the test_range::sentinel >>>type satisfy std::sized_sentinel_for when the iterator satisfies >>>std::random_access_iterator, i.e. support subtracting iterators and >>>sentinels from each other. >>> >>>Tested powerpc64le-linux, committed to trunk. >> >>Thanks for the review and the testsuite fixes. >> >>> >>>Please go ahead and commit your patch to test_range::end() after >>>re-testing. Thanks, and congratulations on your first libstdc++ >>>patch. >> >>Thanks! :) I am re-testing now, and will commit the patch after that's >>done. > >It looks like there are other testsuite failures that need to get >addressed, in particular in the tests >range_operations/{prev,distance,next}.cc. > >In prev.cc and next.cc we are passing a sentinel as the first argument >to ranges::next and ranges::prev, which expect an iterator as their >first argument. In distance.cc we're passing a sentinel as the first >argument to ranges::distance, which also expects an iterator as its >first argument. > >Here's an updated patch that adjusts these tests in the straightforward >way. In test04() in next.cc I had to reset the bounds on the container >after doing ranges::next(begin, end) since we're manipulating an >input_iterator_wrapper. Please add a comment there saying something like // reset single-pass input range so it's obvious why it's needed. >Is this OK to commit? OK for master with that tweak, thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-31 10:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-21 23:23 [PATCH] libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() Patrick Palka 2020-01-29 14:00 ` Jonathan Wakely 2020-01-29 16:43 ` Patrick Palka 2020-01-29 17:35 ` Patrick Palka 2020-01-31 11:20 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).