[libcxx-commits] [PATCH] D126616: [libc++] Implement ranges::move{, _backward}

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_move.h:38-44
+  template <class _InIter, class _Sent, class _OutIter>
+    requires __iter_move::__move_deref<_InIter> // check that we are allowed to std::move() the value
+  _LIBCPP_HIDE_FROM_ABI constexpr static
+  move_result<_InIter, _OutIter> __move_impl(_InIter __first, _Sent __last, _OutIter __result) {
+    auto __ret = std::__move(std::move(__first), std::move(__last), std::move(__result));
+    return {std::move(__ret.first), std::move(__ret.second)};
+  }
----------------
huixie90 wrote:
> philnik wrote:
> > huixie90 wrote:
> > > Why do we need this overload? shouldn't `ranges::move` always call `ranges::iter_move`?
> > This isn't needed, but this allows us to unwrap the iterator and call `memmove` on `trivially_copyable` types.
> But this overload has the wrong behaviour for proxy iterators. For example zip_view
> 
> see this example: https://godbolt.org/z/fWG6Te69T
> 
> if you call ranges::move on two `zip_view`s, because `zip_view`'s reference type is `tuple<X&>`, `std::move(tuple<X&>)` gives you `tuple<X&>&&>`, which will still trigger copy assignment of `X`. but `zip_view` customises its `iter_move` to return `tuple<X&&>` so if you use `iter_move(it)`, it would trigger the move assignment of `X`.
> 
> I think correctness takes precedence of performance.
That's why it's guarded with `requires __iter_move::__move_deref<_InIter>` to ensure that `ranges::iter_move` would just call `std::move`. Or am I missing something? Your example also looks correct to me: https://godbolt.org/z/vjchvc11z.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126616/new/

https://reviews.llvm.org/D126616