[libcxx-commits] [PATCH] D127557: [libc++][ranges] Implement `ranges::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.

I really like the approach of using a custom comparator to apply the projections! That makes the implementation so much simpler than the way I would have done it. It already looks quite good, although I'm fearing that the custom comparator interferes with the branchless sorting for arithmetic types. If that is the case it should show up in the benchmarks.



================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:76
 Permutation,stable_partition,Not assigned,n/a,Not started
-Permutation,sort,Konstantin Varlamov,n/a,In progress
+Permutation,sort,`TODO <https://llvm.org/TODO>`_,n/a,Under review
 Permutation,stable_sort,Konstantin Varlamov,n/a,In progress
----------------
A nice checkbox for a TODO


================
Comment at: libcxx/include/__algorithm/ranges_sort.h:37
+struct __fn {
+  template <random_access_iterator _Iter, sentinel_for<_Iter> _Sent, class _Comp = ranges::less, class _Proj = identity>
+    requires sortable<_Iter, _Comp, _Proj>
----------------
Just a thought, and this should probably be done in a follow-up patch: Do we want to specialize this if `is_same_v<_Comp, ranges::less> && is_same_v<_Proj, identity>`? That is probably the normal use case and it would avoid code duplication. `sort` is quite large.


================
Comment at: libcxx/include/__algorithm/ranges_sort.h:41-45
+    auto __projected_comp = [&](auto&& __lhs, auto&& __rhs) {
+      return std::invoke(__comp,
+                         std::invoke(__proj, std::forward<decltype(__lhs)>(__lhs)),
+                         std::invoke(__proj, std::forward<decltype(__rhs)>(__rhs)));
+    };
----------------
This is valid because `sortable` guarantees that the same object with different cv-ref qualifications compares identical. To be precise, `sortable` -> `strict_weak_order` -> `relation` -> `predicate` is the chain to follow for this information from https://en.cppreference.com/w/cpp/iterator/sortable.


================
Comment at: libcxx/include/__algorithm/ranges_sort.h:47
+
+    if constexpr (std::same_as<_Iter, _Sent>) {
+      std::__sort_impl(std::move(__first), __last, __projected_comp);
----------------
This is unnecessary. `ranges::next` already checks for `assignable_from<_Ip&, _Sp>` and in that case just returns the sentinel.


================
Comment at: libcxx/include/__algorithm/ranges_sort.h:62
+  borrowed_iterator_t<_Range> operator()(_Range&& __r, _Comp __comp = {}, _Proj __proj = {}) const {
+    return (*this)(ranges::begin(__r), ranges::end(__r), __comp, __proj);
+  }
----------------
This copies the comparator and projection. This should be catched by `ranges_robust_against_copying_{comparators, projections}.pass.cpp`.


================
Comment at: libcxx/include/__algorithm/sort.h:576
   _LIBCPP_DEBUG_RANDOMIZE_RANGE(__first, __last);
-  typedef typename __comp_ref_type<_Compare>::type _Comp_ref;
+  typedef typename __comp_ref_type<_Comp>::type _Comp_ref;
   if (__libcpp_is_constant_evaluated()) {
----------------
`using`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Could you also add a benchmark for `ranges::sort` and make sure the numbers are within margin of error compared to `std::sort`? It would be really bad if the ranges version performed worse than the classic STL version.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:18
+//   constexpr I
+//     sort(I first, S last, Comp comp = {}, Proj proj = {});                                // since C++20
+//
----------------
I don't think we're adding this to the synopsis in tests, but I don't really care either way.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:41-46
+static_assert(HasSortIt<int*>);
+static_assert(!HasSortIt<RandomAccessIteratorNotDerivedFrom>);
+static_assert(!HasSortIt<RandomAccessIteratorBadIndex>);
+static_assert(!HasSortIt<int*, SentinelForNotSemiregular>);
+static_assert(!HasSortIt<int*, SentinelForNotWeaklyEqualityComparableWith>);
+static_assert(!HasSortIt<int*, int*, BadComparator>);
----------------
I think you are missing SFINAE checks for `sortable`. I think it could be as simple as `static_assert(!HasSortIt<const int*>)` ditto for the range version.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:75
+
+    std::same_as<Iter> decltype(auto) last = std::ranges::sort(std::ranges::subrange(b, e));
+    assert(sorted == expected);
----------------
This specifically checks with a borrowing range. I think I'd rather give it a lvalue-range and put the borrowing part into it's own test.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:142
+      std::array in = {A{2}, A{3}, A{1}};
+      std::ranges::sort(in.begin(), in.end(), std::less{}, &A::a);
+      assert((in == std::array{A{1}, A{2}, A{3}}));
----------------
Is there a reason you aren't just using `{}` for the comparator?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:148
+      std::array in = {A{2}, A{3}, A{1}};
+      std::ranges::sort(in, std::less{}, &A::a);
+      assert((in == std::array{A{1}, A{2}, A{3}}));
----------------
Optional: Could you also check the returned iterator in all cases?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:161
+
+      constexpr bool operator==(S rhs) const { return i == rhs.i; }
+    };
----------------
I //think// this should work. Same for you `A` struct above.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/ranges.sort.pass.cpp:180
+  }
+
+  return true;
----------------
You are missing a convertible to bool test.


================
Comment at: libcxx/test/support/almost_satisfies_types.h:330-335
+  friend bool operator==(const Self&, const Self&);
+  friend bool operator!=(const Self&, const Self&);
+  friend bool operator< (const Self&, const Self&);
+  friend bool operator<=(const Self&, const Self&);
+  friend bool operator> (const Self&, const Self&);
+  friend bool operator>=(const Self&, const Self&);
----------------
Couldn't you simply use `std::strong_ordering operator<=>(const Self&) const noexcept;`?


================
Comment at: libcxx/test/support/almost_satisfies_types.h:383
+template <class Iter>
+class MoveOnlyComparator {
+public:
----------------
I don't think there are any algorithms where you can pass a move-only comparator. Could you rename this to something like `ComparatorNotCopyable`? That would describe better what you are checking IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127557