This patch implements operator<=> for std::reverse_iterator and
also adds a test that checks that three-way comparison of different
instantiations of std::reverse_iterator works as expected (related to
D113417).
Details
- Reviewers
ldionne • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rGd8f3cdfed03f: [libcxx] Implement three-way comparison for std::reverse_iterator
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp | ||
---|---|---|
59 | Nit: we can drop the second argument here to static_assert if you'd like. |
Looks acceptable as-is, but if we can convince you to scope-creep it to adding C++20 constraints on the comparison operators, that'd be cool. :)
libcxx/include/__iterator/reverse_iterator.h | ||
---|---|---|
194 | C++20 also adds constraints to all six of these operators, e.g.:
Scope-creep: shall we add these constraints right now? E.g. template <class _Iter1, class _Iter2> inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 bool operator<=(const reverse_iterator<_Iter1>& __x, const reverse_iterator<_Iter2>& __y) #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS) requires is_convertible_v<decltype(x.base() >= y.base()), bool> #endif { return __x.base() >= __y.base(); } | |
libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp | ||
32 | The only reason to template this on ItL and ItR is if you're going to test different iterator types. Specifically, let's test one where its comparison category is partial_ordering, to make sure that that comes through OK. (I wonder if iota_view<float>::iterator exists and if so what its comparison category is.) |
libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp | ||
---|---|---|
32 |
Yes, in this case, char * and const char *.
OK, I will add such a test. |
LGTM mod comments. Guess I failed to nerdsnipe you into adding the constraints to the other six relational operators, huh?
libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp | ||
---|---|---|
47 | https://quuxplusone.github.io/blog/2021/07/14/an-iterator-is-not-a-const-iterator/ ConstIter(Iter it): m_value(it.m_value) {} right? Just write that one line. :) Note that if these were real iterators, in C++20, inheritance would totally not work, because you'd have signatures like ConstIter& Iter::operator++() which wouldn't satisfy the iterator concepts. Inheritance and value-semantic types really do not mix. | |
57–59 | Style nit: Both operator== and operator<=> should be hidden friends of ConstIter. UB nit: Your operator== is not consistent with your operator<=>. I suggest just declaring double m_value, and defaulting both == and <=>. Then you can use ConstIter(std::numeric_limits<double>::quiet_NaN()) as your "unordered" iterator value. (And I recommend pulling out a variable double nan = std::numeric_limits<double>::quiet_NaN(); in main for readability.) |
libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp | ||
---|---|---|
47 |
Thanks, now I finally got it. |
Guess I failed to nerdsnipe you into adding the constraints to the other six relational operators, huh?
I'll submit a separate patch
LGTM! Please wait for second libc++ approval before landing.
libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp | ||
---|---|---|
34 | Add a blank line between lines 34 and 35. | |
87 | This is a known bug in GCC constexpr evaluation. You can just change this from Iter(nan), Iter(6) to Iter(nan), Iter(nan) and that should make GCC happy. |
libcxx/include/__iterator/reverse_iterator.h | ||
---|---|---|
198 |
Nope, I disagree. It's just really unhelpful, but I think you are missing some includes.
Note that D114010 will make that error useful again, but it's blocked on a couple things.
C++20 also adds constraints to all six of these operators, e.g.:
Scope-creep: shall we add these constraints right now? E.g.