[libcxx-commits] [PATCH] D113417: [libcxx] Fix enable_if condition of std::reverse_iterator::operator=

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:114
         is_convertible<_Up const&, _Iter>::value &&
-        is_assignable<_Up const&, _Iter>::value
+        is_assignable<_Iter, _Up const&>::value
     > >
----------------
C++17 actually doesn't constrain this `operator=` at all
https://timsong-cpp.github.io/cppwp/n4659/reverse.iter.op=
but C++20 added constraints
https://cplusplus.github.io/LWG/issue3435
which @ldionne added (in all modes) in e4d3a993c2675f46cbe99acd1bf6e6d39d9c1aee.
I believe we should make this constraint apply only in C++20, and follow the Standard's subtly different spec: `convertible_to, assignable_from` instead of `is_convertible, is_assignable`.

Scope creep: Any appetite for adding `operator<=>` at the same time, so that we could actually verify that `three_way_comparable_with<reverse_iterator<int*>, reverse_iterator<const int*>>` will be true after this patch? (I believe the only actually missing puzzle piece is `operator<=>`.)


================
Comment at: libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp:67
+    rev_ti = rev_fi;
+}
+
----------------
Very nice catch!
As always, I'm not thrilled about misusing inheritance like this. I see that inheriting `to_iter : const_bidir_iter` is a cheap way to get the right Big Five iterator typedefs for your type, but it doesn't actually make `to_iter` into a bidirectional iterator (i.e. `std::bidirectional_iterator<to_iter> == false`), because it leaves its `operator++` with the wrong return type, and so on. Meanwhile, the inheritance makes overload resolution more confusing than it needs to be.
But, at the same time, I'm not vehemently convinced that something manual like https://godbolt.org/z/bqM5hMEd4 would really be any better. :)

I believe `test_assign_convertible()` can easily be made constexpr-friendly and shoved into `tests()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113417