[libc++] Fix LWG3390: move_iterator now handles move-only iterators.

[• Quuxplusone]

Summary

This can't really be tested until C++20 move_iterator is completely implemented, but I'm working on that.

Depends on D117324.

[ldionne]

This can't really be tested until C++20 move_iterator is completely implemented, but I'm working on that.

Can't we add a simple test with a move_iterator constructed from a non-copyable iterator and ensure that works? What else is missing?

This LGTM but I'd like to see tests for this, so I'll wait to see the child revisions I assume you're about to upload.

[• Quuxplusone]

@ldionne: I've uploaded my proposed test now, but as it turns out, it fails because move_it.base() returns by-value in C++17; actually pulling out the .base() of a move-only move_iterator requires the C++20 changes to make .base() return by-const-ref.

I'd still prefer to land this as-is (without that test for now), but I guess I could also abandon this. My original rationale for opening this individually was that this is a (minor) change to the pre-C++20 behavior — it now moves instead of copying. I think everything else I'm doing affects our behavior only in C++20-and-later.

(Okay, so technically this behavior is observable and therefore testable pre-C++20; I'd just have to make a local iterator type that is both copy-ctable and move-ctable, but its copy-ctor counts the number of times it's called, and verify that make_move_iterator(CountingItType()) never calls the copy-ctor. But that seems like overkill, IMVHO.)

[ldionne]

Ahh I see. Fine with this as-is without the test. I like to have a single commit for this LWG issue. My understanding is that you will then test that (and more) in D117656.

libcxx/docs/Status/Cxx20Issues.csv

 "`3381 <https://wg21.link/LWG3381>`__","``begin``\  and ``data``\  must agree for ``contiguous_range``\ ","Prague","","","|ranges|"
 "`3382 <https://wg21.link/LWG3382>`__","NTTP for ``pair``\  and ``array``\ ","Prague","",""
 "`3383 <https://wg21.link/LWG3383>`__","|sect|\ [time.zone.leap.nonmembers] ``sys_seconds``\  should be replaced with ``seconds``\ ","Prague","","","|chrono|"
 "`3384 <https://wg21.link/LWG3384>`__","``transform_view::*sentinel*``\  has an incorrect ``operator-``\ ","Prague","","","|ranges|"
 "`3385 <https://wg21.link/LWG3385>`__","``common_iterator``\  is not sufficiently constrained for non-copyable iterators","Prague","","","|ranges|"
 "`3387 <https://wg21.link/LWG3387>`__","|sect|\ [range.reverse.view] ``reverse_view<V>``\  unintentionally requires ``range<const V>``\ ","Prague","","","|ranges|"
 "`3388 <https://wg21.link/LWG3388>`__","``view``\  iterator types have ill-formed ``<=>``\  operators","Prague","","","|ranges|"
 "`3389 <https://wg21.link/LWG3389>`__","A move-only iterator still does not have a ``counted_iterator``\ ","Prague","","","|ranges|"
-"`3390 <https://wg21.link/LWG3390>`__","``make_move_iterator()``\  cannot be used to construct a ``move_iterator``\  for a move-only iterator","Prague","","","|ranges|"
+"`3390 <https://wg21.link/LWG3390>`__","``make_move_iterator()``\  cannot be used to construct a ``move_iterator``\  for a move-only iterator","Prague","|Complete|","14.0","|ranges|"
 "`3393 <https://wg21.link/LWG3393>`__","Missing/incorrect feature test macro for coroutines","Prague","|Complete|","14.0"
 "`3395 <https://wg21.link/LWG3395>`__","Definition for three-way comparison needs to be updated (US 152)","Prague","","","|spaceship|"
 "`3396 <https://wg21.link/LWG3396>`__","Clarify point of reference for ``source_location::current()``\  (DE 169)","Prague","",""
 "`3397 <https://wg21.link/LWG3397>`__","``ranges::basic_istream_view::iterator``\  should not provide ``iterator_category``\ ","Prague","","","|ranges|"
 "`3398 <https://wg21.link/LWG3398>`__","``tuple_element_t``\  is also wrong for ``const subrange``\ ","Prague","|Complete|","14.0","|ranges|"
 "`3446 <https://wg21.link/LWG3446>`__","``indirectly_readable_traits``\ ambiguity for types with both ``value_type``\ and ``element_type``\ ","November virtual meeting","|Complete|","13.0"

libcxx/include/__iterator/move_iterator.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___ITERATOR_MOVE_ITERATOR_H
 #define _LIBCPP___ITERATOR_MOVE_ITERATOR_H
 
 #include <__config>
 #include <__iterator/iterator_traits.h>
+#include <__utility/move.h>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 

 #else
     typedef typename iterator_traits<iterator_type>::reference reference;
 #endif
 
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
     move_iterator() : __current_() {}
 
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
-    explicit move_iterator(_Iter __i) : __current_(__i) {}
+    explicit move_iterator(_Iter __i) : __current_(_VSTD::move(__i)) {}
 
     template <class _Up, class = __enable_if_t<
         !is_same<_Up, _Iter>::value && is_convertible<const _Up&, _Iter>::value
     > >
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
     move_iterator(const move_iterator<_Up>& __u) : __current_(__u.base()) {}
 
     template <class _Up, class = __enable_if_t<
operator+(typename move_iterator<_Iter>::difference_type __n, const move_iterator<_Iter>& __x)
     return move_iterator<_Iter>(__x.base() + __n);
 }
 
 template <class _Iter>
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
 move_iterator<_Iter>
 make_move_iterator(_Iter __i)
 {
-    return move_iterator<_Iter>(__i);
+    return move_iterator<_Iter>(_VSTD::move(__i));
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ITERATOR_MOVE_ITERATOR_H

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

 // move_iterator
 
 // explicit move_iterator(Iter i);
 //
 //  constexpr in C++17
 
 #include <iterator>
 #include <cassert>
+#include <utility>
 
 #include "test_macros.h"
 #include "test_iterators.h"
 
 template <class It>
-void
-test(It i)
+TEST_CONSTEXPR_CXX17 bool test()
 {
-    std::move_iterator<It> r(i);
-    assert(r.base() == i);
+  static_assert( std::is_constructible<std::move_iterator<It>, const It&>::value, "");
+  static_assert( std::is_constructible<std::move_iterator<It>, It&&>::value, "");
+  static_assert(!std::is_convertible<const It&, std::move_iterator<It>>::value, "");
+  static_assert(!std::is_convertible<It&&, std::move_iterator<It>>::value, "");
+
+  char s[] = "123";
+  {
+    It it = It(s);
+    std::move_iterator<It> r(it);
+    assert(base(r.base()) == s);
+  }
+  {
+    It it = It(s);
+    std::move_iterator<It> r(std::move(it));
+    assert(base(r.base()) == s);
+  }
+  return true;
 }
 
-int main(int, char**)
+template <class It>
+TEST_CONSTEXPR_CXX17 bool test_moveonly()
 {
-    char s[] = "123";
-    test(cpp17_input_iterator<char*>(s));
-    test(forward_iterator<char*>(s));
-    test(bidirectional_iterator<char*>(s));
-    test(random_access_iterator<char*>(s));
-    test(s);
+  static_assert(!std::is_constructible<std::move_iterator<It>, const It&>::value, "");
+  static_assert( std::is_constructible<std::move_iterator<It>, It&&>::value, "");
+  static_assert(!std::is_convertible<const It&, std::move_iterator<It>>::value, "");
+  static_assert(!std::is_convertible<It&&, std::move_iterator<It>>::value, "");
 
-#if TEST_STD_VER > 14
-    {
-    constexpr const char *p = "123456789";
-    constexpr std::move_iterator<const char *> it(p);
-    static_assert(it.base() == p);
-    }
+  char s[] = "123";
+    It it = It(s);
+    std::move_iterator<It> r(std::move(it));
+    assert(base(r.base()) == s);
+  }
+  return true;
+
+int main(int, char**)
+{
+  test<cpp17_input_iterator<char*> >();
+  test<forward_iterator<char*> >();
+  test<bidirectional_iterator<char*> >();
+  test<random_access_iterator<char*> >();
+  test<char*>();
+  test<const char*>();
+
+#if TEST_STD_VER > 14
+  static_assert(test<cpp17_input_iterator<char*>>());
+  static_assert(test<forward_iterator<char*>>());
+  static_assert(test<bidirectional_iterator<char*>>());
+  static_assert(test<random_access_iterator<char*>>());
+  static_assert(test<char*>());
+  static_assert(test<const char*>());
+#endif
+
+#if TEST_STD_VER > 17
+  test<contiguous_iterator<char*>>();
+  test_moveonly<cpp20_input_iterator<char*>>();
+  static_assert(test<contiguous_iterator<char*>>());
+  static_assert(test_moveonly<cpp20_input_iterator<char*>>());
 #endif
 
   return 0;
 }