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

Wed Jun 8 16:17:45 PDT 2022

philnik added inline comments.

================
Comment at: libcxx/include/__algorithm/move.h:49
+#ifndef _LIBCPP_COMPILER_GCC
+   && !is_trivially_copyable<_InValueT>::value
+#endif
----------------
var-const wrote:
> Can you provide a bit more context about this workaround?
What are you asking about exactly? Why I have to check for `is_trivially_copyable`? That's because `__builtin_memmove` requires the type to be trivially_copable. Or do you mean something else?

================
Comment at: libcxx/include/__algorithm/move.h:54
+  const size_t __n = static_cast<size_t>(__last - __first);
+  if (__n > 0)
+    ::__builtin_memmove(__result, __first, __n * sizeof(_OutValueT));
----------------
var-const wrote:
> Is this check necessary? Does `__builtin_memmove` require the argument not to be zero?
AFAIK `__builtin_memmove` doesn't require the argument to be non-zero. The compiler doesn't eliminate the check either so I'm not sure if it should stay or not. This means potentially less function call, but also a branch more. I don't think it makes a large difference either way, so I'll remove it.

================
Comment at: libcxx/include/__algorithm/move.h:74
+                             && __is_trivially_move_assignable_unwrapped<_InIter>::value
+                             && __is_trivially_move_assignable_unwrapped<_OutIter>::value> >
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
----------------
var-const wrote:
> Question: if `_InIter` and `_OutIter` have the same underlying value type, wouldn't `__is_trivially_move_assignable_unwrapped` always return the same result for both of them?
`__is_trivially_move_assignable_unwrapped` also checks that we get a raw pointer. But that's not relevant, since I'm updating the check to be equivalent to the check in `copy`.

================
Comment at: libcxx/include/__algorithm/move.h:77
+pair<reverse_iterator<_InIter>, reverse_iterator<_OutIter>>
+__move_impl(reverse_iterator<_InIter> __first,
+            reverse_iterator<_InIter> __last,
----------------
var-const wrote:
> Question: this is a new optimization that isn't directly related to implementing `ranges::move`, right?
It's a new optimization for `std::move`, but I'm removing a specialization from `std::move_backward` that would have done the same thing. Without this 'new' optimization I would pessimize `std::move_backward` and `ranges::move_backward`. I also think it's a good idea to implement more algorithms in terms of other algorithms to get more optimizations and get the optimizations into a single place instead of duplicating them all over the place. To achieve that I'm using `reverse_iterator` in quite a few places, so it's getting a lot more likely to have weird things like `reverse_iterator<reverse_iterator<T>>` or even more wrapped around.

================
Comment at: libcxx/include/__algorithm/move.h:85
+  std::__move_impl(__last_base, __first_base, __result_first);
+  return std::make_pair(__last, reverse_iterator<_OutIter>(std::__rewrap_iter(__result.base(), __result_first)));
+}
----------------
var-const wrote:
> Very optional: you can use `make_reverse_iterator`.
`make_reverse_iterator` requires C++14 and I don't think it's worth to add a new `__make_reverse_iterator` for relatively little extra convenience.

================
Comment at: libcxx/include/__algorithm/move.h:91
+pair<reverse_iterator<reverse_iterator<_InIter>>, reverse_iterator<reverse_iterator<_OutIter> > >
+__copy_impl(reverse_iterator<reverse_iterator<_InIter> > __first,
+            reverse_iterator<reverse_iterator<_InIter> > __last,
----------------
var-const wrote:
> Is this used? (same for this function's implementation below)
This has been superseded by D127049. Removing it.

================
Comment at: libcxx/include/__algorithm/move.h:126

 template <class _InputIterator, class _OutputIterator>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
var-const wrote:
> Nit: this should be `_InIter` for consistency (I'd personally prefer `_InputIter`, but feel free to ignore).
Nope. This is one of the classic STL algorithms where we use `_InputIterator` and friends. This is specifically to make them easily distinguishable.

================
Comment at: libcxx/include/__algorithm/move_backward.h:28
 {
-    if (__libcpp_is_constant_evaluated()) {
-        return _VSTD::__move_backward_constexpr(__first, __last, __result);
-    } else {
-        return _VSTD::__rewrap_iter(__result,
-            _VSTD::__move_backward(_VSTD::__unwrap_iter(__first),
-                                   _VSTD::__unwrap_iter(__last),
-                                   _VSTD::__unwrap_iter(__result)));
-    }
+  return std::__move(reverse_iterator<_BidirectionalIterator1>(__last),
+                     reverse_iterator<_BidirectionalIterator1>(__first),
----------------
var-const wrote:
> I'd rather keep the previous approach -- it's more verbose, but also straightforward, symmetrical with `move.h`, and avoiding the extra overhead from using reverse iterators (which includes the additional mental burden when reading the code and more complicated types when debugging).
I strongly disagree. It's currently a lot of code duplication without any obvious benefit IMO. I don't see how any symmetry is good here. We already have a better implementation of the algorithm, so why not use it? I also don't understand what mental burden you mean. I think it's a lot easier to reason about a forward going algorithm. If that works, a `reverse_iterator` call to that also works. With a `reverse_iterator` the types are technically more complicated, but how often do you step into `move_backward`? Almost never I would think. Forwarding to `std::move` also allows code sharing if you have `std::move(reverse_iterator<T>)` and `std::move_backward(T)` and similar combinations.

================
Comment at: libcxx/include/__algorithm/ranges_move.h:40
+  template <class _InIter, class _Sent, class _OutIter>
+    requires __iter_move::__move_deref<_InIter>
+  _LIBCPP_HIDE_FROM_ABI constexpr static
----------------
var-const wrote:
> Question: can you provide more context on this constraint?
As you can see below, `ranges::move` is required to use `ranges::iter_move`. The overload with `__move_deref<_Iter>` is the one that uses `std::move(*__iter)` (effectively). I //think// this can also be enabled in the `__just_deref` case, but I'm not sure and I wanted to play it safe for now.

================
Comment at: libcxx/include/__algorithm/ranges_move_backward.h:45
+  move_backward_result<_InIter, _OutIter> __move_backward_impl(_InIter __first, _Sent __last, _OutIter __result) {
+    auto __ret = std::__move(std::make_reverse_iterator(ranges::next(__first, __last)),
+                             std::make_reverse_iterator(__first),
----------------
var-const wrote:
> Why does this function forward to `__move` with reverse iterators rather than `__move_backward`? (I understand that `__move_backward` would have to be added) While this would be more boilerplate, I think it would be simpler, more consistent, and avoid the extra overhead from using reverse iterators (and probably the need to treat them specially in `__move`).
See my comment in `move_backward.h`.

================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:38
+struct NotIndirectlyCopyable {};
+static_assert(!HasMoveIt<int*, NotIndirectlyCopyable*>);
+static_assert(!HasMoveIt<int*, int*, SentinelForNotSemiregular>);
----------------
var-const wrote:
> Question: are these types for checking the `weakly_incrementable` constraint? Just clarifying because the names seem unrelated.
Nope, that's for checking the `requires` clause. I just forgot to rename it to `NotIndirectlyMovable` though. `weakly_incrementable` is checked by `WeaklyIncrementableNotMovable`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126616