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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri Jun 10 08:59:00 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list