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

Nothing major, but I'd like to take another look after my comments have been addressed.



================
Comment at: libcxx/include/__algorithm/ranges_stable_sort.h:42-43
   template <class _Iter, class _Sent, class _Comp, class _Proj>
-  _LIBCPP_HIDE_FROM_ABI constexpr static
-  _Iter __sort_fn_impl(_Iter __first, _Sent __last, _Comp& __comp, _Proj& __proj) {
+  _LIBCPP_HIDE_FROM_ABI
+  _Iter __stable_sort_fn_impl(_Iter __first, _Sent __last, _Comp& __comp, _Proj& __proj) const {
     auto __last_iter = ranges::next(__first, __last);
----------------
Why is this not `static` but instead marked `const`?


================
Comment at: libcxx/include/__algorithm/ranges_stable_sort.h:43
+  _LIBCPP_HIDE_FROM_ABI
+  _Iter __stable_sort_fn_impl(_Iter __first, _Sent __last, _Comp& __comp, _Proj& __proj) const {
     auto __last_iter = ranges::next(__first, __last);
----------------
Maybe name it `__ranges_stable_sort_impl`? 


================
Comment at: libcxx/include/__algorithm/ranges_stable_sort.h:46
 
     auto&& __projected_comp = __make_projected_comp(__comp, __proj);
+    std::__stable_sort_impl(std::move(__first), __last_iter, __projected_comp);
----------------
This probably also applies to `ranges::sort`.


================
Comment at: libcxx/include/__algorithm/stable_sort.h:212
+  if (__len > static_cast<difference_type>(__stable_sort_switch<value_type>::value)) {
+      __buf = std::get_temporary_buffer<value_type>(__len);
+      __h.reset(__buf.first);
----------------
Would you be interested in removing `std::get_temporary_buffer` and `std::return_temporary_buffer` in a follow-up patch? It's deprecated and just forwards to `new` and `delete`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:129-132
+      bool operator<(const OrderedValue& rhs) const { return value < rhs.value; }
+      bool operator<=(const OrderedValue& rhs) const { return value <= rhs.value; }
+      bool operator>(const OrderedValue& rhs) const { return value > rhs.value; }
+      bool operator>=(const OrderedValue& rhs) const { return value >= rhs.value; }
----------------
As always, I think this //should// work. I'm not yet fluent in spaceship operator.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:137
+    using Array = std::array<V, 20>;
+    Array orig_in = {
+      V{10, 101}, {12, 121}, {3, 31}, {5, 51}, {3, 32}, {3, 33}, {11, 111}, {12, 122}, {4, 41}, {4, 42}, {4, 43},
----------------
Does it not work to just use `std::array orig_in = {V{10, 101}, ...}`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:187
+      int a;
+      bool operator==(const A&) const = default;
+    };
----------------
Isn't this be unnecessary and even counter-productive?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:213
+
+      bool operator==(const S&) const = default;
+    };
----------------
Again, I don't think this should be here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127834