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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 3 inline comments as done.
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:
> > > 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
Ah OK. I can see that the comment could be interpreted the wrong way. If you're writing a comment yourself it often feels a lot more obvious than it actually is.
I too think that I could also enable it for `__just_deref`, but I wasn't 100% sure if I'm correct on that, so I wanted to postpone this optimziation to a follow-up PR. Calling `std::move` either way is a lot easier to ensure than checking all the implications of casting a type to an rvalue.
Anyways, I'm planning to look into that optimization and I'll update the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126616