This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Implement three-way comparison for std::reverse_iterator
ClosedPublic

Authored by miyuki on Nov 11 2021, 10:35 AM.

Details

Summary

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).

Diff Detail

Event Timeline

miyuki requested review of this revision.Nov 11 2021, 10:35 AM
miyuki created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2021, 10:35 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser added a subscriber: jloser.Nov 11 2021, 10:41 AM
jloser added inline comments.
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.:

Constraints: x.base() == y.base() is well-formed and convertible to bool.

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.)

miyuki added inline comments.Nov 12 2021, 3:08 AM
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.

Yes, in this case, char * and const char *.

Specifically, let's test one where its comparison category is partial_ordering

OK, I will add such a test.

miyuki updated this revision to Diff 387255.Nov 15 2021, 7:32 AM

Added std::partial_ordering test

miyuki marked 2 inline comments as done.Nov 15 2021, 7:33 AM
Quuxplusone requested changes to this revision.Nov 15 2021, 10:27 AM

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
46

https://quuxplusone.github.io/blog/2021/07/14/an-iterator-is-not-a-const-iterator/
All you're trying to do here is create a conversion

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.

56–58

Style nit: Both operator== and operator<=> should be hidden friends of ConstIter.
operator== can be defaulted.

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.)

This revision now requires changes to proceed.Nov 15 2021, 10:27 AM
miyuki updated this revision to Diff 388884.Nov 22 2021, 5:00 AM
miyuki set the repository for this revision to rG LLVM Github Monorepo.

Updated the test.

miyuki marked 2 inline comments as done.Nov 22 2021, 5:02 AM
miyuki added inline comments.
libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp
46

https://quuxplusone.github.io/blog/2021/07/14/an-iterator-is-not-a-const-iterator/
std::vector<int>::iterator b;
std::vector<int>::const_iterator& rb = b; // 1

Thanks, now I finally got it.

miyuki marked an inline comment as done.Nov 22 2021, 5:02 AM

Guess I failed to nerdsnipe you into adding the constraints to the other six relational operators, huh?

I'll submit a separate patch

miyuki updated this revision to Diff 389751.Nov 25 2021, 5:39 AM

Added missing constexpr

LGTM! Please wait for second libc++ approval before landing.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp
35

Add a blank line between lines 34 and 35.

88

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.

ldionne accepted this revision.Nov 25 2021, 6:48 AM
ldionne added inline comments.
libcxx/include/__iterator/reverse_iterator.h
198
This revision is now accepted and ready to land.Nov 25 2021, 6:48 AM
miyuki updated this revision to Diff 389800.Nov 25 2021, 7:53 AM

Addressed comments.

miyuki marked 3 inline comments as done.Nov 25 2021, 7:53 AM

The CI failure looks unrelated.

The CI failure looks unrelated.

Nope, I disagree. It's just really unhelpful, but I think you are missing some includes.

The CI failure looks unrelated.

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.

miyuki updated this revision to Diff 389847.Nov 25 2021, 10:56 AM

Added missing #include-s

ldionne accepted this revision.Nov 25 2021, 12:16 PM

🚢 it!