[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