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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 7 inline comments as done.
var-const added a comment.

@philnik I have moved the changes to `iterator_operations.h` to https://reviews.llvm.org/D129390 and addressed the related feedback there.

It's true that we can go for the more generic version later, but it also gives me a certain peace of mind that no matter what kind of issues may come up from sharing the same implementation, they can be worked around (even if they turn out not to be related to iterator operations).



================
Comment at: libcxx/include/__algorithm/iterator_operations.h:68
+  static typename iterator_traits<typename remove_reference<_Iter>::type>::value_type&& iter_move(_Iter&& __i) {
+    return std::move(*std::forward<_Iter>(__i));
+  }
----------------
philnik wrote:
> Why is the `std::forward` required?
This is to make the implementation as similar as possible to the actual C++20 `iter_move`. Perhaps because the iterator type could have overloads of `operator*` based on whether it's an lvalue or an rvalue? FWIW, the One Ranges Proposal doesn't have `forward`, so it's something that must have come up during subsequent discussions. Regardless of the original motivation, it seems easier to keep it for consistency with the actual function -- what do you think?


================
Comment at: libcxx/include/__algorithm/lower_bound.h:30
 
-template <class _IterOps, class _Iter, class _Sent, class _Type, class _Proj, class _Comp>
+template <class _Tag, class _Iter, class _Sent, class _Type, class _Proj, class _Comp>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
philnik wrote:
> The name `_Tag` looks quite opaque to me. How about `_Namespace` or something like that?
How about something like `_AlgFamily` or `_InvocationContext`?


================
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:
> var-const wrote:
> > 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)?
> I updated D129099 to fix the first overload. could you please double check if that is OK?
Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744