From 42cda3ba45fca30e73e1c35d8e19b5ec8af24d98 Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Sat, 28 Mar 2020 21:52:13 +0000 Subject: [PATCH] libstdc++: Fix std::reverse_iterator relational operators My recent changes to reverse_iterator's comparisons was not the version of the code (or tests) that I meant to commit, and broke the relational operators. This fixes them to reverse the order of the comparisons on the base() iterators. This also replaces the SFINAE constraints in the return type of the reverse_iterator and move_iterator comparisons with a requires-clause. This ensures the constrained overloads are preferred to unconstrained ones. This means the non-standard same-type overloads can be omitted for C++20 because they're not needed to solve the problem with std::rel_ops or the testsuite's greedy_ops::X type. * include/bits/stl_iterator.h (reverse_iterator): Use requires-clause to constrain C++20 versions of comparison operators. Fix backwards logic of relational operators. (move_iterator): Use requires-clause to constrain comparison operators in C++20. Do not declare non-standard same-type overloads for C++20. * testsuite/24_iterators/move_iterator/rel_ops_c++20.cc: Check result of comparisons and check using greedy_ops type. * testsuite/24_iterators/reverse_iterator/rel_ops_c++20.cc: Likewise. * testsuite/24_iterators/move_iterator/greedy_ops.cc: Remove redundant main function from compile-only test. * testsuite/24_iterators/reverse_iterator/greedy_ops.cc: Likewise. --- libstdc++-v3/ChangeLog | 14 ++ libstdc++-v3/include/bits/stl_iterator.h | 136 ++++++++++-------- .../24_iterators/move_iterator/greedy_ops.cc | 8 +- .../move_iterator/rel_ops_c++20.cc | 34 +++++ .../reverse_iterator/greedy_ops.cc | 8 +- .../reverse_iterator/rel_ops_c++20.cc | 61 ++++++-- 6 files changed, 172 insertions(+), 89 deletions(-) diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index 3b96b23ca8f..a016f640df3 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,3 +1,17 @@ +2020-03-28 Jonathan Wakely + + * include/bits/stl_iterator.h (reverse_iterator): Use requires-clause + to constrain C++20 versions of comparison operators. Fix backwards + logic of relational operators. + (move_iterator): Use requires-clause to constrain comparison operators + in C++20. Do not declare non-standard same-type overloads for C++20. + * testsuite/24_iterators/move_iterator/rel_ops_c++20.cc: Check result + of comparisons and check using greedy_ops type. + * testsuite/24_iterators/reverse_iterator/rel_ops_c++20.cc: Likewise. + * testsuite/24_iterators/move_iterator/greedy_ops.cc: Remove redundant + main function from compile-only test. + * testsuite/24_iterators/reverse_iterator/greedy_ops.cc: Likewise. + 2020-03-27 Jonathan Wakely * include/bits/range_cmp.h (__cpp_lib_ranges): Define. diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h index e68f66a2b89..d7972b71998 100644 --- a/libstdc++-v3/include/bits/stl_iterator.h +++ b/libstdc++-v3/include/bits/stl_iterator.h @@ -341,9 +341,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return __t.operator->(); } }; - // Used in unevaluated expressions to test for implicit conversion to bool. - namespace __detail { bool __convbool(bool); } - //@{ /** * @param __x A %reverse_iterator. @@ -354,7 +351,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * iterators. * */ -#if __cplusplus <= 201703L +#if __cplusplus <= 201703L || ! defined __cpp_lib_concepts template inline _GLIBCXX17_CONSTEXPR bool operator==(const reverse_iterator<_Iterator>& __x, @@ -430,46 +427,53 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return !(__x < __y); } #else // C++20 template - constexpr auto + constexpr bool operator==(const reverse_iterator<_IteratorL>& __x, const reverse_iterator<_IteratorR>& __y) - -> decltype(__detail::__convbool(__x.base() == __y.base())) + requires requires { { __x.base() == __y.base() } -> convertible_to; } { return __x.base() == __y.base(); } template - constexpr auto + constexpr bool operator!=(const reverse_iterator<_IteratorL>& __x, const reverse_iterator<_IteratorR>& __y) - -> decltype(__detail::__convbool(__x.base() != __y.base())) + requires requires { { __x.base() != __y.base() } -> convertible_to; } { return __x.base() != __y.base(); } template - constexpr auto + constexpr bool operator<(const reverse_iterator<_IteratorL>& __x, const reverse_iterator<_IteratorR>& __y) - -> decltype(__detail::__convbool(__x.base() < __y.base())) - { return __x.base() < __y.base(); } - - template - constexpr auto - operator>(const reverse_iterator<_IteratorL>& __x, - const reverse_iterator<_IteratorR>& __y) - -> decltype(__detail::__convbool(__x.base() > __y.base())) + requires requires { { __x.base() > __y.base() } -> convertible_to; } { return __x.base() > __y.base(); } template - constexpr auto - operator<=(const reverse_iterator<_IteratorL>& __x, - const reverse_iterator<_IteratorR>& __y) - -> decltype(__detail::__convbool(__x.base() <= __y.base())) - { return __x.base() <= __y.base(); } + constexpr bool + operator>(const reverse_iterator<_IteratorL>& __x, + const reverse_iterator<_IteratorR>& __y) + requires requires { { __x.base() < __y.base() } -> convertible_to; } + { return __x.base() < __y.base(); } template - constexpr auto + constexpr bool + operator<=(const reverse_iterator<_IteratorL>& __x, + const reverse_iterator<_IteratorR>& __y) + requires requires { { __x.base() >= __y.base() } -> convertible_to; } + { return __x.base() >= __y.base(); } + + template + constexpr bool operator>=(const reverse_iterator<_IteratorL>& __x, const reverse_iterator<_IteratorR>& __y) - -> decltype(__detail::__convbool(__x.base() >= __y.base())) - { return __x.base() >= __y.base(); } + requires requires { { __x.base() <= __y.base() } -> convertible_to; } + { return __x.base() <= __y.base(); } + + template _IteratorR> + constexpr compare_three_way_result_t<_IteratorL, _IteratorR> + operator<=>(const reverse_iterator<_IteratorL>& __x, + const reverse_iterator<_IteratorR>& __y) + { return __y.base() <=> __x.base(); } #endif // C++20 //@} @@ -1413,24 +1417,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #endif // C++20 }; - // Note: See __normal_iterator operators note from Gaby to understand - // why there are always 2 versions for most of the move_iterator - // operators. template inline _GLIBCXX17_CONSTEXPR bool operator==(const move_iterator<_IteratorL>& __x, const move_iterator<_IteratorR>& __y) #if __cplusplus > 201703L && __cpp_lib_concepts - requires requires { __detail::__convbool(__x.base() == __y.base()); } + requires requires { { __x.base() == __y.base() } -> convertible_to; } #endif { return __x.base() == __y.base(); } - template - inline _GLIBCXX17_CONSTEXPR bool - operator==(const move_iterator<_Iterator>& __x, - const move_iterator<_Iterator>& __y) - { return __x.base() == __y.base(); } - #if __cpp_lib_three_way_comparison template _IteratorR> @@ -1444,12 +1439,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION operator!=(const move_iterator<_IteratorL>& __x, const move_iterator<_IteratorR>& __y) { return !(__x == __y); } - - template - inline _GLIBCXX17_CONSTEXPR bool - operator!=(const move_iterator<_Iterator>& __x, - const move_iterator<_Iterator>& __y) - { return !(__x == __y); } #endif template @@ -1457,60 +1446,81 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION operator<(const move_iterator<_IteratorL>& __x, const move_iterator<_IteratorR>& __y) #if __cplusplus > 201703L && __cpp_lib_concepts - requires requires { __detail::__convbool(__x.base() < __y.base()); } + requires requires { { __x.base() < __y.base() } -> convertible_to; } #endif { return __x.base() < __y.base(); } - template - inline _GLIBCXX17_CONSTEXPR bool - operator<(const move_iterator<_Iterator>& __x, - const move_iterator<_Iterator>& __y) - { return __x.base() < __y.base(); } - template inline _GLIBCXX17_CONSTEXPR bool operator<=(const move_iterator<_IteratorL>& __x, const move_iterator<_IteratorR>& __y) #if __cplusplus > 201703L && __cpp_lib_concepts - requires requires { __detail::__convbool(__y.base() < __x.base()); } + requires requires { { __y.base() < __x.base() } -> convertible_to; } #endif { return !(__y < __x); } - template - inline _GLIBCXX17_CONSTEXPR bool - operator<=(const move_iterator<_Iterator>& __x, - const move_iterator<_Iterator>& __y) - { return !(__y < __x); } - template inline _GLIBCXX17_CONSTEXPR bool operator>(const move_iterator<_IteratorL>& __x, const move_iterator<_IteratorR>& __y) #if __cplusplus > 201703L && __cpp_lib_concepts - requires requires { __detail::__convbool(__y.base() < __x.base()); } + requires requires { { __y.base() < __x.base() } -> convertible_to; } #endif { return __y < __x; } - template - inline _GLIBCXX17_CONSTEXPR bool - operator>(const move_iterator<_Iterator>& __x, - const move_iterator<_Iterator>& __y) - { return __y < __x; } - template inline _GLIBCXX17_CONSTEXPR bool operator>=(const move_iterator<_IteratorL>& __x, const move_iterator<_IteratorR>& __y) #if __cplusplus > 201703L && __cpp_lib_concepts - requires requires { __detail::__convbool(__x.base() < __y.base()); } + requires requires { { __x.base() < __y.base() } -> convertible_to; } #endif { return !(__x < __y); } +#if ! (__cplusplus > 201703L && __cpp_lib_concepts) + // Note: See __normal_iterator operators note from Gaby to understand + // why we have these extra overloads for some move_iterator operators. + + // These extra overloads are not needed in C++20, because the ones above + // are constrained with a requires-clause and so overload resolution will + // prefer them to greedy unconstrained function templates. + + template + inline _GLIBCXX17_CONSTEXPR bool + operator==(const move_iterator<_Iterator>& __x, + const move_iterator<_Iterator>& __y) + { return __x.base() == __y.base(); } + + template + inline _GLIBCXX17_CONSTEXPR bool + operator!=(const move_iterator<_Iterator>& __x, + const move_iterator<_Iterator>& __y) + { return !(__x == __y); } + + template + inline _GLIBCXX17_CONSTEXPR bool + operator<(const move_iterator<_Iterator>& __x, + const move_iterator<_Iterator>& __y) + { return __x.base() < __y.base(); } + + template + inline _GLIBCXX17_CONSTEXPR bool + operator<=(const move_iterator<_Iterator>& __x, + const move_iterator<_Iterator>& __y) + { return !(__y < __x); } + + template + inline _GLIBCXX17_CONSTEXPR bool + operator>(const move_iterator<_Iterator>& __x, + const move_iterator<_Iterator>& __y) + { return __y < __x; } + template inline _GLIBCXX17_CONSTEXPR bool operator>=(const move_iterator<_Iterator>& __x, const move_iterator<_Iterator>& __y) { return !(__x < __y); } +#endif // ! C++20 // DR 685. template diff --git a/libstdc++-v3/testsuite/24_iterators/move_iterator/greedy_ops.cc b/libstdc++-v3/testsuite/24_iterators/move_iterator/greedy_ops.cc index 66da0a4cff9..0e6d598cd43 100644 --- a/libstdc++-v3/testsuite/24_iterators/move_iterator/greedy_ops.cc +++ b/libstdc++-v3/testsuite/24_iterators/move_iterator/greedy_ops.cc @@ -24,7 +24,7 @@ void test01() typedef std::move_iterator iterator_type; iterator_type it(nullptr); - + it == it; it != it; it < it; @@ -35,9 +35,3 @@ void test01() 1 + it; it + 1; } - -int main() -{ - test01(); - return 0; -} diff --git a/libstdc++-v3/testsuite/24_iterators/move_iterator/rel_ops_c++20.cc b/libstdc++-v3/testsuite/24_iterators/move_iterator/rel_ops_c++20.cc index 4e7b9d01e15..a530923f8ad 100644 --- a/libstdc++-v3/testsuite/24_iterators/move_iterator/rel_ops_c++20.cc +++ b/libstdc++-v3/testsuite/24_iterators/move_iterator/rel_ops_c++20.cc @@ -127,3 +127,37 @@ static_assert( has_le<2> ); static_assert( ! has_ge<0> ); static_assert( has_ge<1> ); static_assert( ! has_ge<2> ); + +int arr[3] = { 1, 2, 3 }; +constexpr std::move_iterator beg{std::begin(arr)}; +constexpr std::move_iterator cbeg{std::cbegin(arr)}; +static_assert( beg == cbeg ); +static_assert( beg <= cbeg ); +static_assert( beg >= cbeg ); +static_assert( std::is_eq(beg <=> cbeg) ); +constexpr std::move_iterator cend{std::cend(arr)}; +static_assert( beg != cend ); +static_assert( beg < cend ); +static_assert( cend > beg ); +static_assert( beg <= cend ); +static_assert( cend >= beg ); +static_assert( std::is_lt(beg <=> cend) ); + +#include + +void test01() +{ + typedef std::move_iterator iterator_type; + + iterator_type it(nullptr); + + it == it; + it != it; + it < it; + it <= it; + it > it; + it >= it; + // it - it; // See PR libstdc++/71771 + 1 + it; + it + 1; +} diff --git a/libstdc++-v3/testsuite/24_iterators/reverse_iterator/greedy_ops.cc b/libstdc++-v3/testsuite/24_iterators/reverse_iterator/greedy_ops.cc index 60d4611e690..a5ea4524604 100644 --- a/libstdc++-v3/testsuite/24_iterators/reverse_iterator/greedy_ops.cc +++ b/libstdc++-v3/testsuite/24_iterators/reverse_iterator/greedy_ops.cc @@ -24,7 +24,7 @@ void test01() typedef std::reverse_iterator iterator_type; iterator_type it; - + it == it; it != it; it < it; @@ -37,9 +37,3 @@ void test01() 1 + it; it + 1; } - -int main() -{ - test01(); - return 0; -} diff --git a/libstdc++-v3/testsuite/24_iterators/reverse_iterator/rel_ops_c++20.cc b/libstdc++-v3/testsuite/24_iterators/reverse_iterator/rel_ops_c++20.cc index 3e91a0396fe..a382ae52483 100644 --- a/libstdc++-v3/testsuite/24_iterators/reverse_iterator/rel_ops_c++20.cc +++ b/libstdc++-v3/testsuite/24_iterators/reverse_iterator/rel_ops_c++20.cc @@ -78,10 +78,10 @@ reverse_iterator r{nullptr}; bool b0 = l0 == r; bool b1 = l1 != r; -bool b2 = l2 < r; -bool b3 = l3 > r; -bool b4 = l4 <= r; -bool b5 = l5 >= r; +bool b2 = l2 > r; +bool b3 = l3 < r; +bool b4 = l4 >= r; +bool b5 = l5 <= r; template concept has_eq @@ -129,15 +129,15 @@ static_assert( ! has_ne<5> ); static_assert( ! has_lt<0> ); static_assert( ! has_lt<1> ); -static_assert( has_lt<2> ); -static_assert( ! has_lt<3> ); +static_assert( ! has_lt<2> ); +static_assert( has_lt<3> ); static_assert( ! has_lt<4> ); static_assert( ! has_lt<5> ); static_assert( ! has_gt<0> ); static_assert( ! has_gt<1> ); -static_assert( ! has_gt<2> ); -static_assert( has_gt<3> ); +static_assert( has_gt<2> ); +static_assert( ! has_gt<3> ); static_assert( ! has_gt<4> ); static_assert( ! has_gt<5> ); @@ -145,12 +145,49 @@ static_assert( ! has_le<0> ); static_assert( ! has_le<1> ); static_assert( ! has_le<2> ); static_assert( ! has_le<3> ); -static_assert( has_le<4> ); -static_assert( ! has_le<5> ); +static_assert( ! has_le<4> ); +static_assert( has_le<5> ); static_assert( ! has_ge<0> ); static_assert( ! has_ge<1> ); static_assert( ! has_ge<2> ); static_assert( ! has_ge<3> ); -static_assert( ! has_ge<4> ); -static_assert( has_ge<5> ); +static_assert( has_ge<4> ); +static_assert( ! has_ge<5> ); + +int arr[3] = { 1, 2, 3 }; +constexpr std::reverse_iterator rbeg = std::rbegin(arr); +constexpr std::reverse_iterator crbeg = std::crbegin(arr); +static_assert( rbeg == crbeg ); +static_assert( rbeg <= crbeg ); +static_assert( rbeg >= crbeg ); +static_assert( std::is_eq(rbeg <=> crbeg) ); +constexpr std::reverse_iterator crend = std::crend(arr); +static_assert( rbeg != crend ); +static_assert( rbeg < crend ); +static_assert( crend > rbeg ); +static_assert( rbeg <= crend ); +static_assert( crend >= rbeg ); +static_assert( std::is_lt(rbeg <=> crend) ); + +#include + +// copied from 24_iterators/reverse_iterator/greedy_ops.cc +void test01() +{ + typedef std::reverse_iterator iterator_type; + + iterator_type it; + + it == it; + it != it; + it < it; + it <= it; + it > it; + it >= it; +#if __cplusplus < 201103L + it - it; // See PR libstdc++/71771 +#endif + 1 + it; + it + 1; +}