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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/basic_operations.h:30
+template <>
+struct _BasicOperations<_STLClassicAlgorithms> {
+  // iter_swap
----------------
philnik wrote:
> Isn't this pretty much exactly the same as `_StdIterOps` and `_RangesIterOps` (in `__algorithm/iterator_operations.h`), just with a bit more boiler-plate?
Sorry, I completely missed that patch. You're right, it's pretty much the same idea. The only difference of any significance is that a tag can be used to make arbitrary decisions -- or rather, passing a generic tag instead of a class containing a set of operations makes it clearer that this is intended to be a generic mechanism, not limited to just iterator operations. I could imagine something like:
```
template <class _Tag, class _Iter>
decltype(auto) __foo_impl(_Iter __b, _Iter __e) {
  // ...
  if constexpr (same_as<_Tag, _RangeAlgorithms>)
    return __e;
}
```
(Of course, this could be done as `if constexpr(same_as<_IterOps, _RangesIterOps>)` too, but I think it is a bit less readable)

In either case, there's definitely duplication here. Let's discuss whether being more generic with tags is worth the boilerplate, and I'll consolidate the approach in this patch (whichever we decide).

(By the way, the `static constexpr auto advance = ranges::advance;` idea is pretty neat)


================
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:
> 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.


================
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
----------------
huixie90 wrote:
> 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?
This is a great catch, thanks! Do you plan to fix this in D129099 (since that patch already contains an overload of `__make_projected_comp` that avoids the issue)?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:284
+}
+
+int main(int, char**) {
----------------
huixie90 wrote:
> 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
Not yet -- I plan to add a test once your patch lands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744