[libc++] Fix a typo in reverse_iterator::operator=

[• Quuxplusone]

Summary

We should be checking is_assignable<It&, ...>. is_assignable<It, ...> checks for an rvalue left-hand side, which is basically never assignable-to.
Found while looking into https://cplusplus.github.io/LWG/issue3435 .

I don't think this is worth testing, because it's just a matter of whether we call it.operator=(jt) (correct) or it.operator=(It(jt)) (current behavior). Also, the behavior apparently changes again in C++20. Also, there's implementation divergence because of our dumb _LIBCPP_ABI_NO_ITERATOR_BASES thing (so we make twice as many copies as we need to — btw this is going to be an extra mess as soon as we need to support move-only iterators in reverse_iterator)
https://godbolt.org/z/5TKvshM3f

But I'd like to get the typo-fix landed now, so that it doesn't fall through the cracks.

[CaseyCarter]

We should be checking is_assignable<It&, ...>. is_assignable<It, ...> checks for an rvalue left-hand side, which is basically never assignable-to.

On the contrary, is_assignable<T, U> is almost always equal to is_assignable<T&, U> when T is a class type since assignment operators for class types are rarely &-qualified. I assume that's how this constraint slipped through testing, which indicates lack of test coverage for e.g. reverse_iterator<int*>.

this is going to be an extra mess as soon as we need to support move-only iterators in reverse_iterator

forward_iterator requires copyable. Move-only iterators are necessarily single-pass, and therefore never valid parameters for reverse_iterator.

[• Quuxplusone]

In D117660#3255829, @CaseyCarter wrote:

We should be checking is_assignable<It&, ...>. is_assignable<It, ...> checks for an rvalue left-hand side, which is basically never assignable-to.

On the contrary, is_assignable<T, U> is almost always equal to is_assignable<T&, U> when T is a class type since assignment operators for class types are rarely &-qualified. I assume that's how this constraint slipped through testing, which indicates lack of test coverage for e.g. reverse_iterator<int*>.

It's subtler than that (thank goodness). If this operator= is incorrectly missing, then overload resolution ends up picking the copy assignment operator=, i.e. x.operator=(y) quietly becomes x.operator=(X(y)). And if X is int*, then I don't think that difference is observable... oh, except perhaps via the no-two-implicit-conversions-in-a-row "rule"... Well, I noodled around and got this far, but it still relies on ref-qualified operator=: https://godbolt.org/z/64v93nzPq

this is going to be an extra mess as soon as we need to support move-only iterators in reverse_iterator

forward_iterator requires copyable. Move-only iterators are necessarily single-pass, and therefore never valid parameters for reverse_iterator.

Okay, I buy that. So at least the "extra mess" is not coming in C++20 or (almost certainly) C++23. :)

[ldionne]

I read what you wrote, but adding a test should be really simple, right? Something like this:

struct RefQualifiedIterator {
  template <typename T>
  RefQualifiedIterator(T const&);

  template <typename T>
  RefQualifiedIterator& operator=(T&) &;

  // other iterator members
};

LGTM with or without the test, but I'd add it since it's so simple and it would have caught this bug.

libcxx/include/__iterator/reverse_iterator.h

#ifndef _LIBCPP_ABI_NO_ITERATOR_BASES
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator(const reverse_iterator<_Up>& __u)
         : __t(__u.base()), current(__u.base())
     { }
 
     template <class _Up, class = __enable_if_t<
         !is_same<_Up, _Iter>::value &&
         is_convertible<_Up const&, _Iter>::value &&
-        is_assignable<_Iter, _Up const&>::value
+        is_assignable<_Iter&, _Up const&>::value
     > >
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator& operator=(const reverse_iterator<_Up>& __u) {
         __t = current = __u.base();
         return *this;
     }
 #else
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator() : current() {}
 
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     explicit reverse_iterator(_Iter __x) : current(__x) {}
 
     template <class _Up, class = __enable_if_t<
         !is_same<_Up, _Iter>::value && is_convertible<_Up const&, _Iter>::value
     > >
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator(const reverse_iterator<_Up>& __u)
         : current(__u.base())
     { }
 
     template <class _Up, class = __enable_if_t<
         !is_same<_Up, _Iter>::value &&
         is_convertible<_Up const&, _Iter>::value &&
-        is_assignable<_Iter, _Up const&>::value
+        is_assignable<_Iter&, _Up const&>::value
     > >
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
     reverse_iterator& operator=(const reverse_iterator<_Up>& __u) {
         current = __u.base();
         return *this;
     }
 #endif
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14