[libcxx-commits] [PATCH] D113695: [libcxx] Implement three-way comparison for std::reverse_iterator

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

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



================
Comment at: libcxx/include/__iterator/reverse_iterator.h:194
     return __x.base() >= __y.base();
 }
 
----------------
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();
}
```


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp:31
+    const std::reverse_iterator<ItR> r1(r);
+    ASSERT_SAME_TYPE(decltype(l1 <=> r1), std::strong_ordering);
+    assert((l1 <=> r1) == x);
----------------
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113695/new/

https://reviews.llvm.org/D113695