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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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:
> 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.
> 
I understand Costa's point about debugging and I agree with it to some extent. However, concretely, I don't think users would be stepping through our whole `std::move_backward` implementation anyways, and if so, they are probably placing breakpoints in strategic places in their code to avoid stepping through our implementation details. Hence, I think there are other ways to mitigate the downsides of this implementation, and I think the optimization in `std::move` and the removal of duplication in `std::move_backward` are worth it. Just my .02, but I would err towards the currently-proposed approach. If we gain more experience with this sort of technique (which BTW is akin to expression templates in a really basic way), and if we realize that it has negative effects for our users, we can and should revisit this stance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126616