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

[Hui via Phabricator via libcxx-commits]

huixie90 requested changes to this revision.
huixie90 added inline comments.


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:66-68
+  // Declaring the return type is necessary for the C++03 mode (which doesn't support placeholder return types).
+  static typename iterator_traits<typename remove_reference<_Iter>::type>::value_type&& iter_move(_Iter&& __i) {
+    return std::move(*std::forward<_Iter>(__i));
----------------
FWIW,  `auto` return type and this explicit return type can be different. e.g for `std::vector<bool>::iterator`,   `std::move(*std::forward<_Iter>(__i));` is `proxy&&` and `value_type&&` is `bool&&`. However. in classic algorithm, you have to do write
```
value_type temp = std::move(*it);
```
because
```
auto temp = std::move(*it); 
```
does the wrong thing for `vector<bool>` and friends.

So here this explicit return type does the conversion sometimes and probably IS what the caller wants


================
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
----------------
var-const wrote:
> philnik wrote:
> > The name `_Tag` looks quite opaque to me. How about `_Namespace` or something like that?
> How about something like `_AlgFamily` or `_InvocationContext`?
Or `_Policy` and we have `_RangesPolicy` and `_ClassicPolicy`


================
Comment at: libcxx/include/__algorithm/make_heap.h:30
 {
     typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
     difference_type __n = __last - __first;
----------------
For C++20 algorithms, we should use `iter_difference_t `.

the `difference_type` is coming from `incrementable_traits` when `iterator_traits` is not specialized

(Note this applies to all other usages of `difference_type`)


================
Comment at: libcxx/include/__algorithm/pop_heap.h:33
 {
     using value_type = typename iterator_traits<_RandomAccessIterator>::value_type;
 
----------------
For C++20 ranges algorithms, we should use `iter_value_t`.
because the `value_type` is coming from `indirectly_readable_traits` when `iterator_traits` is not specialized

This applies to all other places we use `value_type`


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/partial.sort/ranges_partial_sort.pass.cpp:281
+  }
+
+  return true;
----------------
btw,  you can rebase and try sorting the `ProxyRange` to verify if `iter_swap` is used properly used and sorting `ProxyRange` of  move only types to see if `iter_move` is properly used


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744