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!
