[libcxx] Fix enable_if condition of std::reverse_iterator::operator=

[miyuki]

Summary

The template std::is_assignable<T, U> checks that T is assignable from
U. Hence, the order of operands in the instantiation of
std::is_assignable in the std::reverse_iterator::operator= condition
should be reversed.

This issue remained unnoticed because std::reverse_iterator has an
implicit conversion constructor. This patch adds a test to check that
the assignment operator is used directly, without any implicit
conversions. The patch also adds a similar test for
std::move_iterator.

[• Quuxplusone]

libcxx/include/__iterator/reverse_iterator.h

114

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

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp

101

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

[ldionne]

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?

libcxx/include/__iterator/reverse_iterator.h

114

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

[miyuki]

1. Updated std::reverse_iterator tests
2. Added a test for std::move_iterator

[miyuki]

libcxx/include/__iterator/reverse_iterator.h

114

Scope creep: Any appetite for adding operator<=>

Submitted as a separate patch: D113695

[• Quuxplusone]

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp

40

I recommend eliminating this typedef. Right now there's a symmetry in the names of FromIter and ToIter but actually the former is not a class type.

73

Why is this not just move_ti = move_fi;?
What you've got won't necessarily work on non-libc++ implementations.
Orthogonally(?), if you want to check that this is really calling the "right" operator= instead of doing an implicit conversion followed by the "wrong" operator=, then you could make those two operators do different things; e.g.

ToIter(const FromIter& src) { assert(false); }
TEST_CONSTEXPR_CXX17 ToIter& operator=(const FromIter& src) { m_value = 1; return *this; }
TEST_CONSTEXPR_CXX17 ToIter& operator=(FromIter&& src) { m_value = 1; return *this; }
[...]
move_ti = move_fi;
assert(move_ti.m_value == 1);
move_ti = std::move(move_fi);
assert(move_ti.m_value == 2);

Ditto in the reverse_iterator test below.

[miyuki]

Updated tests

[miyuki]

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp

73

Why is this not just move_ti = move_fi;

assert(false); would not work in constexpr context, so I used an explicit call of operator= as a workaround (it would fail at compile time without the fix in reverse_iterator.h). I found a simpler workaround for assert(false), so now I can use move_ti = move_fi;.

Orthogonally(?), if you want to check that this is really calling the "right" operator=

An assertion in the conversion constructor should be enough to test the fix.

[miyuki]

The AIX failures seem to be unrelated.

[ldionne]

This LGTM. The CI failures is indeed unrelated to this change, so this should be safe to check in.

Please give a bit of time to @Quuxplusone to say if he's still got comments on this, but otherwise let's go ahead. Thanks a lot for fixing this.

@daltenty Can you please take a look at the AIX bots, the seem to have started failing over the night: https://buildkite.com/llvm-project/libcxx-ci/builds/6599#a2b5b4eb-95e7-44bc-b7e1-549367a4be52

[daltenty]

@daltenty Can you please take a look at the AIX bots, the seem to have started failing over the night: https://buildkite.com/llvm-project/libcxx-ci/builds/6599#a2b5b4eb-95e7-44bc-b7e1-549367a4be52

@ldionne Taking a look now, suspect some host machine updates changes something locale wise.

[• Quuxplusone]

LGTM mod a few last comments on the test.

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp

50–53

I believe a better fix would be to remove TEST_CONSTEXPR_CXX17 from this function:

ToIter(char *) { assert(false); }

This function should never be called at all. You could even leave off the body if you want:

ToIter(char *);

(But you can't =delete it, because the library is checking that is_convertible_v<char*, ToIter>.)

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp

43–48

Ditto here: just ToIter(char *); should suffice.

[miyuki]

Rebased and addressed Arthur's comment.

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<_Up const&, _Iter>::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<_Up const&, _Iter>::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

libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp

test(U u)
     std::move_iterator<It>& rr = (r1 = r2);
     assert(r1.base() == u);
     assert(&rr == &r1);
 }
 
 struct Base {};
 struct Derived : Base {};
 
+struct ToIter {
+    typedef std::forward_iterator_tag iterator_category;
+    typedef char *pointer;
+    typedef char &reference;
+    typedef char value_type;
+    typedef value_type difference_type;
+
+    explicit TEST_CONSTEXPR_CXX17 ToIter() : m_value(0) {}
+    TEST_CONSTEXPR_CXX17 ToIter(const ToIter &src) : m_value(src.m_value) {}
+    TEST_CONSTEXPR_CXX17 ToIter(char *src) : m_value(src) {
+        bool f = false;
+        // assert(false) will not work in constexpr context, so we use a
+        // variable to workaround this issue
+        assert(f);
+    }
+    TEST_CONSTEXPR_CXX17 ToIter &operator=(char *src) {
+        m_value = src;
+        return *this;
+    }
+    TEST_CONSTEXPR_CXX17 ToIter &operator=(const ToIter &src) {
+        m_value = src.m_value;
+        return *this;
+    }
+    char *m_value;
+};
+
+TEST_CONSTEXPR_CXX17 bool test_conv_assign()
+{
+    char c = '\0';
+    char *fi = &c;
+    const std::move_iterator<char *> move_fi(fi);
+    std::move_iterator<ToIter> move_ti;
+    move_ti = move_fi;
+    assert(move_ti.base().m_value == fi);
+    return true;
+}
+
 int main(int, char**)
 {
     Derived d;
 
     test<cpp17_input_iterator<Base*> >(cpp17_input_iterator<Derived*>(&d));
     test<forward_iterator<Base*> >(forward_iterator<Derived*>(&d));
     test<bidirectional_iterator<Base*> >(bidirectional_iterator<Derived*>(&d));
     test<random_access_iterator<const Base*> >(random_access_iterator<Derived*>(&d));
     test<Base*>(&d);
+    test_conv_assign();
 #if TEST_STD_VER > 14
     {
     using BaseIter    = std::move_iterator<const Base *>;
     using DerivedIter = std::move_iterator<const Derived *>;
     constexpr const Derived *p = nullptr;
     constexpr DerivedIter     it1 = std::make_move_iterator(p);
     constexpr BaseIter        it2 = (BaseIter{nullptr} = it1);
     static_assert(it2.base() == p, "");
+    static_assert(test_conv_assign(), "");
     }
 #endif
 
   return 0;
 }

libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp

TEST_CONSTEXPR_CXX17 void test(U u) {
     std::reverse_iterator<It>& rr = r1 = r2;
     assert(r1.base() == u);
     assert(&rr == &r1);
 }
 
 struct Base { };
 struct Derived : Base { };
 
+struct ToIter {
+    typedef std::bidirectional_iterator_tag iterator_category;
+    typedef char *pointer;
+    typedef char &reference;
+    typedef char value_type;
+    typedef value_type difference_type;
+
+    explicit TEST_CONSTEXPR_CXX17 ToIter() : m_value(0) {}
+    TEST_CONSTEXPR_CXX17 ToIter(const ToIter &src) : m_value(src.m_value) {}
+    TEST_CONSTEXPR_CXX17 ToIter(char *src) : m_value(src) {
+        bool f = false;
+        // assert(false) will not work in constexpr context, so we use a
+        // variable to workaround this issue
+        assert(f);
+    }
+    TEST_CONSTEXPR_CXX17 ToIter &operator=(char *src) {
+        m_value = src;
+        return *this;
+    }
+    TEST_CONSTEXPR_CXX17 ToIter &operator=(const ToIter &src) {
+        m_value = src.m_value;
+        return *this;
+    }
+    char *m_value;
+};
+
 TEST_CONSTEXPR_CXX17 bool tests() {
     Derived d;
     test<bidirectional_iterator<Base*> >(bidirectional_iterator<Derived*>(&d));
     test<random_access_iterator<const Base*> >(random_access_iterator<Derived*>(&d));
     test<Base*>(&d);
+
+    char c = '\0';
+    char *fi = &c;
+    const std::reverse_iterator<char *> rev_fi(fi);
+    std::reverse_iterator<ToIter> rev_ti;
+    rev_ti = rev_fi;
+    assert(rev_ti.base().m_value == fi);
+
     return true;
 }
 
 int main(int, char**) {
     tests();
 #if TEST_STD_VER > 14
     static_assert(tests(), "");
 #endif
     return 0;
 }