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

[Mikhail Maltsev via libcxx-commits]

Author: Mikhail Maltsev
Date: 2021-11-15T13:08:36Z
New Revision: 6938270fa68d2242999802fdc306dc60326df020

URL: https://github.com/llvm/llvm-project/commit/6938270fa68d2242999802fdc306dc60326df020
DIFF: https://github.com/llvm/llvm-project/commit/6938270fa68d2242999802fdc306dc60326df020.diff

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

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.

Reviewed By: Quuxplusone, ldionne, #libc

Differential Revision: https://reviews.llvm.org/D113417

Added: 
    

Modified: 
    libcxx/include/__iterator/reverse_iterator.h
    libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp
    libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp

Removed: 
    


################################################################################
diff  --git a/libcxx/include/__iterator/reverse_iterator.h b/libcxx/include/__iterator/reverse_iterator.h
index fad9bc175b0c..f7a948950df2 100644
--- a/libcxx/include/__iterator/reverse_iterator.h
+++ b/libcxx/include/__iterator/reverse_iterator.h
@@ -86,7 +86,7 @@ _LIBCPP_SUPPRESS_DEPRECATED_POP
     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) {
@@ -111,7 +111,7 @@ _LIBCPP_SUPPRESS_DEPRECATED_POP
     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) {

diff  --git a/libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp b/libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp
index 9b5441431aa6..c6dcef9a0d19 100644
--- a/libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp
+++ b/libcxx/test/std/iterators/predef.iterators/move.iterators/move.iter.ops/move.iter.op=/move_iterator.pass.cpp
@@ -37,6 +37,39 @@ test(U u)
 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 
diff erence_type;
+
+    explicit TEST_CONSTEXPR_CXX17 ToIter() : m_value(0) {}
+    TEST_CONSTEXPR_CXX17 ToIter(const ToIter &src) : m_value(src.m_value) {}
+    // Intentionally not defined, must not be called.
+    ToIter(char *src);
+    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;
@@ -46,6 +79,7 @@ int main(int, char**)
     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 *>;
@@ -54,6 +88,7 @@ int main(int, char**)
     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
 

diff  --git a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp
index 95c6bde040ab..be7f01c16810 100644
--- a/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp
+++ b/libcxx/test/std/iterators/predef.iterators/reverse.iterators/reverse.iter.cons/assign.pass.cpp
@@ -31,11 +31,41 @@ TEST_CONSTEXPR_CXX17 void test(U u) {
 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 
diff erence_type;
+
+    explicit TEST_CONSTEXPR_CXX17 ToIter() : m_value(0) {}
+    TEST_CONSTEXPR_CXX17 ToIter(const ToIter &src) : m_value(src.m_value) {}
+    // Intentionally not defined, must not be called.
+    ToIter(char *src);
+    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;
 }