libstdc++: Always return a sentinel<I> from __gnu_test::test_range::end()
diff mbox series

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()
Related show

Commit Message

Patrick Palka Jan. 21, 2020, 10:26 p.m. UTC
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(-)

Comments

Jonathan Wakely Jan. 29, 2020, 1:46 p.m. UTC | #1
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.
Patrick Palka Jan. 29, 2020, 3:40 p.m. UTC | #2
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.

>
>
>
Patrick Palka Jan. 29, 2020, 4:24 p.m. UTC | #3
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;
Jonathan Wakely Jan. 31, 2020, 10:57 a.m. UTC | #4
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!

Patch
diff mbox series

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;