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

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp:46
+
+struct Iter : ConstIter {
+    using iterator_category = std::bidirectional_iterator_tag;
----------------
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.


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cmp/three-way.pass.cpp:56-58
+constexpr bool operator==(const ConstIter& l, const ConstIter& r) {
+    return l.m_value == r.m_value;
+}
----------------
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.)


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

https://reviews.llvm.org/D113695