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.
commit 5cd2e126f5c5705fa1aa7fafef3d6b94a99593da
Author: Jonathan Wakely <jwak...@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
