[libcxx-commits] [PATCH] D127834: [libc++][ranges] Implement `ranges::stable_sort`.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this! Some small remarks.



================
Comment at: libcxx/include/__algorithm/ranges_stable_sort.h:39
+  template <class _Iter, class _Sent, class _Comp, class _Proj>
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  _Iter __stable_sort_fn_impl(_Iter __first, _Sent __last, _Comp& __comp, _Proj& __proj) const {
----------------
Please remove the `constexpr`


================
Comment at: libcxx/include/__algorithm/stable_sort.h:211
+  if (__len > static_cast<difference_type>(__stable_sort_switch<value_type>::value)) {
+      __buf = _VSTD::get_temporary_buffer<value_type>(__len);
+      __h.reset(__buf.first);
----------------
While you're at it.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:139
+      V{10, 101}, {12, 121}, {3, 31}, {5, 51}, {3, 32}, {3, 33}, {11, 111}, {12, 122}, {4, 41}, {4, 42}, {4, 43},
+      {1, 11}, {6, 61}, {3, 34}, {10, 102}, {8, 81}, {12, 123}, {1, 12}, {1, 13}, {5, 52}
+    };
----------------
I would suggest to make the `original_order` member a simple integer sequence: 1, 2, 3, etc. That makes it easier to verify whether the output is correct. Identical values will have incrementing values.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:150
+      {11, 111},
+      {12, 121}, {12, 122}, {12, 123},
+    };
----------------
Is the trailing comma allowed?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:168
+
+  { // A custom comparator works.
+    {
----------------
It would be good to validate stability in these tests too.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:246
+  }
+
+  { // `std::ranges::dangling` is returned.
----------------
Here we can validate complexity. Obviously implementations can make different choices regarding when there's not "enough extra memory" but we can test the N log (N) for a small number of items.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:255
+  test();
+  // Note: `stable_sort` is not `constexpr`.
+
----------------
Interesting since it's possible to allocate memory in `constexpr` functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127834