[libcxx-commits] [PATCH] D130758: [libc++][ranges] Implement `ranges::rotate`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/algorithm_family.h:58
+  _ForwardIterator2
+  __swap_ranges(_ForwardIterator1 __first1, _ForwardIterator1 __last1, _ForwardIterator2 __first2, _ForwardIterator2) {
+    return std::swap_ranges(std::move(__first1), __last1, std::move(__first2));
----------------
While `swap_ranges` is a very simple algorithm, the range version and the classic version take a different number of parameters (the range version takes `__last2` as well). I can try unifying them but not sure if it's worth pursuing.


================
Comment at: libcxx/include/__algorithm/move.h:114
 _OutputIterator move(_InputIterator __first, _InputIterator __last, _OutputIterator __result) {
-  return std::__move(__first, __last, __result).second;
+  return std::__move<_ClassicAlgPolicy>(__first, __last, __result).second;
 }
----------------
The changes to `move.h` and `move_backward.h` are essentially to pass around `_AlgPolicy` to be able to call `_IterOps::__iter_move` instead of plain `std::move(*iter)`.


================
Comment at: libcxx/include/__algorithm/ranges_move.h:39
   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
----------------
Now that we dispatch to `iter_move`, I think it should take care of this case.


================
Comment at: libcxx/include/__algorithm/rotate.h:197
+  using _Ret = pair<_Iterator, _Iterator>;
+  _Iterator __last_iter = _IterOps<_AlgPolicy>::next(__middle, __last);
+
----------------
The sentinel has to be converted into an iterator early in the call stack to accommodate for the early returns below. This simplifies the other internal functions in this file significantly because they can still accept `__last` as an iterator and return the same type.


================
Comment at: libcxx/include/__iterator/reverse_iterator.h:397
+  _LIBCPP_HIDE_FROM_ABI friend constexpr
+  iter_rvalue_reference_t<_Iter> iter_move(const __unconstrained_reverse_iterator& __i)
+    noexcept(is_nothrow_copy_constructible_v<_Iter> &&
----------------
The lack of `iter_move` was caught by the `robust_against_proxy_iterators` tests in the implementation of `inplace_merge`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130758