| Message ID | 20200121222625.3457862-1-ppalka@gcc.gnu.org |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end() | expand |
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.
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. > > >
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;
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!
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;