[libcxx-commits] [PATCH] D152860: [libc++][PSTL] Implement std::sort

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/pstl_sort.h:31-37
+template <class _ExecutionPolicy,
+          class _RandomAccessIterator,
+          class _Comp                                         = less<>,
+          class _RawPolicy                                    = __remove_cvref_t<_ExecutionPolicy>,
+          enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI void
+sort(_ExecutionPolicy&& __policy, _RandomAccessIterator __first, _RandomAccessIterator __last, _Comp __comp = {}) {
----------------
This way of defining the "two overloads" (the one with and the one without a `comp`) will accept the following code, which shouldn't be valid:

```
std::sort(policy, it1, it2, {});
```

Instead, let's do like we do in the serial `std::sort` and have two separate overloads (and make the no-comp overload simply call the comp overload with `std::less<>()`).

Let's also add a SFINAE test to ensure that we don't accept that code (and while we're at it let's add one to the serial `std::sort` if we don't have one already).


================
Comment at: libcxx/include/__algorithm/pstl_sort.h:40-41
+      _LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_sort),
+      [&__policy](_RandomAccessIterator __g_first, _RandomAccessIterator __g_last, _Comp __g_comp) {
+        std::stable_sort(__policy, __g_first, __g_last, __g_comp);
+      },
----------------
Is there any reason we're not `std::move`ing the arguments like `__g_comp` into `std::stable_sort`? The same observation probably applies to all the other algorithms we have so far.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/sort/pstl.sort.pass.cpp:36
+  void operator()(ExecutionPolicy&& policy) {
+    { // simple test
+      int in[]       = {1, 2, 3, 2, 6, 4};
----------------
I don't think you have any tests for the algorithm with a custom comparator.


================
Comment at: libcxx/test/support/test_macros.h:278
 
+#ifdef _LIBCPP_HAS_NO_RANDOM_DEVICE
+#  define TEST_HAS_NO_RANDOM_DEVICE
----------------
Let's move this closer to line 366 where we define the other similar macros.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152860