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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Please update the ranges status page.



================
Comment at: libcxx/include/__algorithm/move.h:49
+#ifndef _LIBCPP_COMPILER_GCC
+   && !is_trivially_copyable<_InValueT>::value
+#endif
----------------
philnik wrote:
> 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?
I'm asking about the preprocessor guard, why the check is only done if the compiler is not GCC.


================
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));
----------------
philnik wrote:
> 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.
I'd expect `0` to be a very rare case, so I wouldn't add branching to the common case for the sake of it. Moreover, the `0` case will be pretty efficient even without omitting the function call.


================
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,
----------------
philnik wrote:
> 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.
I think this is useful context, consider adding a condensed version of it to the patch description.


================
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)));
+}
----------------
philnik wrote:
> 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.
Makes sense, thanks for explaining.


================
Comment at: libcxx/include/__algorithm/move.h:126
 
 template <class _InputIterator, class _OutputIterator>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
philnik wrote:
> 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.
Ah, I must have missed that part of the discussion, thanks for explaining.


================
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),
----------------
philnik wrote:
> 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.
Let's say I'm writing a custom iterator and get a crash somewhere further down the stack from `move_backward`. The mental burden is having to deal with the additional layer of indirection from `reverse_iterator`. It might be a rare use case, but I don't think we can afford to ignore rare use cases given the size of our user base. Debugging experience is a common complaint about C++.

That said, I understand better why you prefer this approach after seeing the `reverse_copy` implementation which is quite elegant. Since this is a decision that can affect the implementation of quite a few algorithms, I'd ask @ldionne for guidance and defer to his opinion.



================
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
----------------
philnik wrote:
> 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.
Thanks. Can you add a comment like "Checks that `std::move(*__iter)` is well-formed"? I didn't find it entirely obvious from the name, and the implementation checks for a few different things.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/ranges.move.pass.cpp:113
+  test_in_iterators<cpp20_output_iterator<int*>>();
+  test_in_iterators<cpp20_input_iterator<int*>>();
+  test_in_iterators<forward_iterator<int*>>();
----------------
Can you please add a brief comment explaining why `cpp17_input_iterator` doesn't work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126616