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

[Louis Dionne via Phabricator via libcxx-commits]

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

Thanks for catching and fixing this -- how embarrassing! I checked and it looks like `move_iterator` doesn't have the same problem -- would you mind adding a similar test to it just to make sure we don't regress it?



================
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
     > >
----------------
Quuxplusone wrote:
> 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<=>`.)
I added it in all modes because it's a LWG issue, not only a paper. I think that is what we want to do, however you are right we can't implement it exactly per the spec if we backport it to before C++20 (because we don't have concepts then).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113417