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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added subscribers: philnik, ldionne.
ldionne added a comment.

This looks pretty good to me, with some suggestions. I'm especially curious to see what it looks like once we've addressed the issue of using the proper `ranges::` vs `std::` operations (`iter_swap` & friends), as we discussed offline.



================
Comment at: libcxx/include/__algorithm/partial_sort.h:72-78
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
+  _LIBCPP_DEBUG_RANDOMIZE_RANGE(__first, std::ranges::next(__first, __last));
+
+#else
+  static_assert(is_same<_Sentinel, _RandomAccessIterator>::value);
+  _LIBCPP_DEBUG_RANDOMIZE_RANGE(__first, __last);
+#endif
----------------
Instead, I think `_LIBCPP_DEBUG_RANDOMIZE_RANGE` should support `(Iter first, Sentinel last)`. You can call `std::ranges::next` from within `_LIBCPP_DEBUG_RANDOMIZE_RANGE` when `Iter != Sentinel`.

You should wait for @philnik to land his patch about replacing the macro by a function. Also, we should probably move it to a separate header to avoid dragging parts of `<ranges>` in any header that requires `__libcpp_debug_randomize_range` (or whatever the name was).


================
Comment at: libcxx/include/__algorithm/partial_sort.h:83
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+_RandomAccessIterator __partial_sort_impl(_RandomAccessIterator __first, _RandomAccessIterator __middle,
+    _Sentinel __last, _Compare& __comp) {
----------------
I find the layering a bit confusing here. We've got: `partial_sort -> __partial_sort_impl -> __partial_sort`. I would expect either `partial_sort -> __partial_sort -> __partial_sort_impl` (so just swap the names of `__partial_sort` and `__partial_sort_impl`), or you could even inline `__partial_sort_impl` into `__partial_sort` and start calling the latter instead of the former from public functions, since `__partial_sort` is only used here.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:67
+template <class Iter, class Sent, size_t N>
+constexpr void test_one(std::array<int, N> input, size_t mid_index, std::array<int, N> expected) {
+  { // (iterator, sentinel) overload.
----------------
This is not the `expected` sequence since you are only checking a subsequence of it. It's really the full sorted sequence. IMO this renaming (or similar) would make it easier, otherwise one might try to understand why the `expected` sequence has `N` elements when we're partially sorting `mid_index <= N` elements.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:75-76
+    std::same_as<Iter> decltype(auto) last = std::ranges::partial_sort(b, m, e);
+    assert(std::equal(partially_sorted.begin(), partially_sorted.begin() + mid_index, expected.begin(),
+          expected.begin() + mid_index));
+    assert(base(last) == partially_sorted.data() + partially_sorted.size());
----------------
This makes it easier to understand what the `std::equal` is comparing.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:88-89
+    std::same_as<Iter> decltype(auto) last = std::ranges::partial_sort(range, m);
+    assert(std::equal(partially_sorted.begin(), partially_sorted.begin() + mid_index, expected.begin(),
+          expected.begin() + mid_index));
+    assert(base(last) == partially_sorted.data() + partially_sorted.size());
----------------
Same reformatting.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:95
+template <class Iter, class Sent, size_t N>
+constexpr void test_all_cases(std::array<int, N> input) {
+  auto sorted = input;
----------------
Maybe this is closer to what it does? When I think of "cases", I think of what's tested in `check()` below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744