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

[Hui via Phabricator via libcxx-commits]

huixie90 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)};
+  }
----------------
philnik wrote:
> 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.
I see. I read the comments "// check that we are allowed to std::move() the value" and thought it meant that " `std::move(x)` is a valid expression". I think what it actually means is "there is no `iter_move` customisations."

My example is just showing that when there is an `iter_move` customisation, calling `std::move(it, it, o)` is the wrong behaviour.

I think your optimisation is ok but the comment can be made more clearer.

BTW, I think you could also turn on this optimisation for both __move_deref and __just_deref


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126616