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

[Hui via Phabricator via libcxx-commits]

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

Thanks for fixing the `iter_swap` and `iter_move`. It almost LGTM



================
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) {
----------------
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?


================
Comment at: libcxx/include/__algorithm/ranges_basic_operations.h:22
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
----------------
do we need to guard this header with c++ version?


================
Comment at: libcxx/include/__algorithm/ranges_basic_operations.h:33
+  static void __iter_swap(_Iter1&& __a, _Iter2&& __b) {
+    std::ranges::iter_swap(std::forward<_Iter1>(__a), std::forward<_Iter2>(__b));
+  }
----------------
nit: could do `ranges::iter_swap` instead of `std::ranges::iter_swap`


================
Comment at: libcxx/include/__algorithm/sift_down.h:42
 
     if ((__child + 1) < __len && __comp(*__child_i, *(__child_i + difference_type(1)))) {
         // right-child exists and is greater than left-child
----------------
I had a look at the implementation of `_make_projected_comp`, if the new range's algorithm caller pass in a member function pointer and `std::identity` as projection, this `__comp` would be kept as a member function pointer and fail to compile here calling `operator()`

is there any tests covering this case?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:284
+}
+
+int main(int, char**) {
----------------
is there any test case that tests `zip-iterator`-like behavior ?

I am working on a test iterator that does something similar with `zip-iterator` but much simiplied


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744