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

[Nikolas Klauser via Phabricator via libcxx-commits]

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

Could you maybe move the `_IterOps` changes into it's own PR? There are quite a few changes unrelated to `ranges::partial_sort` in here.



================
Comment at: libcxx/include/__algorithm/basic_operations.h:30
+template <>
+struct _BasicOperations<_STLClassicAlgorithms> {
+  // iter_swap
----------------
var-const wrote:
> 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)
FWIW we can still change it later if we want to. But for now, I don't think you are using the `_Tag` for anything other than calling functions from `_BasicOperations`, right? In that case I would definitely go for less boiler-plate. 


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:38
   static constexpr auto distance = ranges::distance;
+  static constexpr auto iter_move = ranges::iter_move;
+  static constexpr auto iter_swap = ranges::iter_swap;
----------------
I think you have to use `__iter_move`, since it doesn't exist pre-C++20. 


================
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));
+  }
----------------
Why is the `std::forward` required?


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:73
+  template <class _Iter1, class _Iter2>
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+  static void iter_swap(_Iter1&& __a, _Iter2&& __b) {
----------------
Why aren't this one and the next also marked `_LIBCPP_CONSTEXPR_AFTER_CXX11`?


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:82
+  static _Iter next(const _Iter& __first, const _Iter& __last) {
+    return std::next(__first, std::distance(__first, __last));
+  }
----------------
huixie90 wrote:
> is it just `return __last`?
+1


================
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
----------------
The name `_Tag` looks quite opaque to me. How about `_Namespace` or something like that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128744