[libcxx-commits] [PATCH] D128744: [libc++][ranges] Implement `ranges::partial_sort`.

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

Mostly nits, but I'd like to see the portability trap addressed. Did you add any `move`s in other classic algorithms? If yes, please also update them.



================
Comment at: libcxx/include/__algorithm/partial_sort.h:56
+
+template <class _AlgPolicy, class _RandomAccessIterator, class _Sentinel>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
Could you add a TODO to remove this and update `__debug_randomize_range` to support ranges once we implement `ranges::shuffle`?


================
Comment at: libcxx/include/__algorithm/partial_sort.h:93-94
 {
-    _VSTD::partial_sort(__first, __middle, __last,
-                        __less<typename iterator_traits<_RandomAccessIterator>::value_type>());
+  std::partial_sort(std::move(__first), std::move(__middle), std::move(__last),
+      __less<typename iterator_traits<_RandomAccessIterator>::value_type>());
 }
----------------
Could you either not move these or `static_assert` that they are copy-constructible and copy-assignable to avoid portability problems?


================
Comment at: libcxx/include/__algorithm/pop_heap.h:58
   typename iterator_traits<_RandomAccessIterator>::difference_type __len = __last - __first;
-  std::__pop_heap(std::move(__first), std::move(__last), __comp, __len);
+  std::__pop_heap<_ClassicAlgPolicy>(std::move(__first), std::move(__last), __comp, __len);
 }
----------------
Pre-existing: Please also remove the `move`s or  add `static_assert`s.


================
Comment at: libcxx/include/__algorithm/push_heap.h:62
 void push_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp) {
-  std::__push_heap(std::move(__first), std::move(__last), __comp);
+  std::__push_heap<_ClassicAlgPolicy>(std::move(__first), std::move(__last), __comp);
 }
----------------
Also here.


================
Comment at: libcxx/include/__algorithm/sort_heap.h:47-48
 void sort_heap(_RandomAccessIterator __first, _RandomAccessIterator __last) {
   std::sort_heap(std::move(__first), std::move(__last),
       __less<typename iterator_traits<_RandomAccessIterator>::value_type>());
 }
----------------
Same here.


================
Comment at: libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp:37
   }
-  std::__partial_sort(v.begin(), v.begin() + kSize / 2, v.end(), std::less<MyType>());
+  std::__partial_sort_impl<std::_STLClassicAlgorithms>(v.begin(), v.begin() + kSize / 2, v.end(), std::less<MyType>());
   return v;
----------------
Shouldn't this be `_ClassicAlgPolicy`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:70-72
+    auto b = Iter(partially_sorted.data());
+    auto m = b + mid_index;
+    auto e = Sent(Iter(partially_sorted.data() + partially_sorted.size()));
----------------
Could you rename these to `begin`, `middle` and `end`? That would make it a lot easier to read IMO.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:88
+    std::same_as<Iter> decltype(auto) last = std::ranges::partial_sort(range, m);
+    assert(std::equal(partially_sorted.begin(), partially_sorted.begin() + mid_index,
+                      sorted.begin(), sorted.begin() + mid_index));
----------------
Why aren't you using `b` and `m` here?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:105-135
+  auto check = []<class Iter, class Sent> {
+    // Empty sequence.
+    test_one<Iter, Sent, 0>({}, 0, {});
+
+    // 1-element sequence.
+    test_all_subsequences<Iter, Sent>(std::array{1});
+
----------------
Could you put this into a function? The calls below look really weird.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:157-159
+      assert(std::ranges::equal(
+          std::ranges::subrange(b, m), std::array{5, 4}
+      ));
----------------
Why aren't these in a single line?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:209-245
+  { // `std::invoke` is used in the implementation.
+    struct S {
+      int i;
+      constexpr S(int i_) : i(i_) {}
+
+      constexpr bool comparator(const S& rhs) const { return i < rhs.i; }
+      constexpr const S& projection() const { return *this; }
----------------
This should be unnecessary now, right?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:247-283
+  { // The comparator can return any type that's convertible to `bool`.
+    const std::array orig_in = {2, 1, 3};
+
+    {
+      auto in = orig_in;
+      auto b = in.begin();
+      auto m = b + 2;
----------------
And this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744