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

Sun Jul 17 20:44:38 PDT 2022

var-const added inline comments.

================
Comment at: libcxx/include/__algorithm/partial_sort.h:37
+_LIBCPP_HIDE_FROM_ABI constexpr
+_RandomAccessIterator __partial_sort_impl(
+    _RandomAccessIterator __first, _RandomAccessIterator __middle, _Sentinel __last, _Compare __comp) {
----------------
huixie90 wrote:
> var-const wrote:
> > huixie90 wrote:
> > > A general question on these `__impl` functions. There are lots of double underscore `impl` functions in the algorithms that are shared between different algorithms. e.g. `__copy`. these functions are not constrained and sometimes I am unsure what condition these functions can be used. I guess some of them are not meant to be used outside the file thus omitting any constraint checks. But it is not easy to find out if it is ok to use these `impl` functions. I am wondering if it is a good idea to add some comments or static_assert on these functions?
> > I think that these functions generally start their life being intended to use only within their file, but later end up being used from outside. The fact that they aren't constrained might be deliberate -- any checks are supposed to have already been performed by the public interface. Comments are definitely useful; `static_assert`s might be useful as well.
> Can we have a naming convention for this? It would be good to distinguish the purpose of `__partial_sort` and `__partial_sort_impl` at the first glance.
> 
>  It would be good to distinguish 
> 1. helper function purely for implementation of the classic algorithm (perhaps with some assumptions that the iterators are classic iterators)
> 2. helper function purely for the implementation of ranges algorithm (perhaps with assumptions that iterators are c++20 iterators)
> 2. helper function that is meant to be shared between the classic algorithm and the ranges algorithm
> 3. helper function can be reused any other algorithms
> 
> The reason why I was asking is that when I implemented other algorithms and I'd like to call `copy`, I was puzzled if I should call `std::__copy` or `std::__copy_impl`
My intention was for `__foo` to be the main internal entry point, i.e., `__foo` functions are the public interface for internal usage, and other algorithms should call into `__foo`. `__foo_impl` would be an implementation detail local to the file (ideally I'd put it into an internal namespace as well).

I'm not sure the finer-grained distinction is necessary -- from what I've seen, we're rewriting all the internal functions to be agnostic to whether they're invoked from a classic or a range algorithm. Please let me know if there's an example where it would be helpful.

Note that in this case, I can't easily combine these two functions -- `__partial_sort` does randomization before and after doing actual work, and the `partial_sort_stability` test relies on being able to call `partial_sort` with and without randomization. If combining the two, one straightforward alternative would be to pass this as a boolean template parameter.

================
Comment at: libcxx/include/__algorithm/partial_sort.h:57
+template <class _AlgPolicy, class _RandomAccessIterator, class _Sentinel>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+void __maybe_randomize(_RandomAccessIterator __first, _Sentinel __last) {
----------------
huixie90 wrote:
> question. why do we need `inline` here as it is already a template (we don't need to worry about ODR violation)?
It was unnecessary, thanks for spotting it.

================
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
----------------
ldionne wrote:
> 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).
Added a TODO -- this will be much easier to implement once `ranges::shuffle` is implemented.

================
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()));
----------------
philnik wrote:
> Could you rename these to `begin`, `middle` and `end`? That would make it a lot easier to read IMO.
Personally, I much prefer the shorter names in cases like this (where the meaning is easy to understand because the pattern of having `begin`/`end` is so ubiquitous, and shorter names help cut down on verbosity). However, it's not worth spending much time debating this, so I renamed like you suggested.

================
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));
----------------
philnik wrote:
> Why aren't you using `b` and `m` here?
Because of `std::equal` -- perhaps `ranges::equal` was unavailable when I was first writing this. In any case, changed to use `ranges::equal` and `begin`/`end`.

================
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});
+
----------------
philnik wrote:
> Could you put this into a function? The calls below look really weird.
This was suggested by @ldionne. The advantage is to have all the related code in the same scope, i.e. to avoid the details spilling out of the function.

================
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}
+      ));
----------------
philnik wrote:
> Why aren't these in a single line?
I find it easier to read this way. Reverted.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744