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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/algorithm_family.h:32
 template <>
 struct _AlgFamily<_RangeAlgPolicy> {
   static constexpr auto __move = ranges::move;
----------------
Doesn't it become possible to remove this entirely now that `__move` accepts a `_AlgPolicy`? I guess we'd have to also accept an `_AlgPolicy` in `__swap_ranges`, and then we can get rid of this and use `__swap_ranges<_AlgPolicy>` and `__move<_AlgPolicy>` instead?


================
Comment at: libcxx/include/__algorithm/algorithm_family.h:34-35
   static constexpr auto __move = ranges::move;
+  // TODO: if using the return type of `ranges::swap_ranges`, note that `result.out` can be different from what would
+  // be returned by `std::swap_ranges`.
+  static constexpr auto __swap_ranges = ranges::swap_ranges;
----------------
Non blocking, but I don't understand the purpose of this TODO. Isn't that already the case with `__move` right above?


================
Comment at: libcxx/include/__algorithm/rotate.h:192
 
-template <class _AlgPolicy, class _RandomAccessIterator, class _IterCategory>
+template <class _AlgPolicy, class _Iterator, class _Sentinel, class _IterCategory>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
Why not do `class _IterCategory = _IterOps<_AlgPolicy>::__iterator_category` since you added it to `_IterOps`? Then it doesn't need to be passed explicitly when calling `__rotate` from `rotate()`, but also from `ranges::rotate` and from `inplace_merge`.

In fact, I think you can straight up just call `__rotate_impl` with the category without even making it a template argument of `__rotate`.


================
Comment at: libcxx/include/__algorithm/rotate.h:200-201
+  if (__first == __middle) {
+      _Iterator __result = _IterOps<_AlgPolicy>::next(__first, __last);
+      return _Ret(std::move(__result), std::move(__last_iter));
+  }
----------------
Couldn't we instead return `_Ret(__last_iter, __last_iter)`? That would save us from advancing the iterator all the way to `__last` twice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130758