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

Mon Jun 27 18:51:22 PDT 2022

var-const added inline comments.

================
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);
----------------
philnik wrote:
> Why is this not `static` but instead marked `const`?
Thanks for spotting!

================
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);
----------------
philnik wrote:
> Maybe name it `__ranges_stable_sort_impl`? 
I somewhat prefer the current version (name of the algorithm + `fn` to indicate that the function is contained within the function object + `impl` for disambiguation). I'd keep the current state unless you feel strongly about it.

================
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);
----------------
philnik wrote:
> This probably also applies to `ranges::sort`.
Thanks! Fixed both.

================
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);
----------------
philnik wrote:
> 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`.
Yes, I can do this in a follow-up (unless you beat me to it :) ).

================
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},
----------------
philnik wrote:
> Does it not work to just use `std::array orig_in = {V{10, 101}, ...}`?
It does -- it just gives an easy way to make sure both arrays have the same number of elements (since they are long, it's easier to make a mistake).

================
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;
+    };
----------------
philnik wrote:
> Isn't this be unnecessary and even counter-productive?
It's used to compare check the `in` array after it's sorted:
```
assert((in == std::array{A{1}, A{2}, A{3}}));
```
Perhaps I'm missing something?

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

================
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}
+    };
----------------
Mordante wrote:
> var-const wrote:
> > Mordante wrote:
> > > 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.
> > I started with that, but then I had the idea of enumerating each duplicate value instead (read those `31`, `32`, `33` as `3.1`, `3.2`, `3.3`, etc.). I think it makes it more readable because following the numbering within a duplicate is much easier than following the numbering across all the elements. What do you think?
> How do you feel about making the `original_order` a double and use `3.1`?
> 
> Since the same constants written the same in the source there shouldn't be an issue with floating point rounding.
I really like this, thanks -- makes the intention a lot clearer and looks better, too.

================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:246
+  }
+
+  { // `std::ranges::dangling` is returned.
----------------
Mordante wrote:
> var-const wrote:
> > Mordante wrote:
> > > 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.
> > Similar to the `sort` patch, I don't think we have a great way of doing this:
> > 
> > - we can use the exact number of operations our implementation happens to perform (we have some precedent in e.g. `test/libcxx/algorithms/alg.sorting/alg.heap.operations/make.heap/complexity.pass.cpp`), but that would make it libc++-specific and, IMO, has limited usefulness;
> > - simply testing for `num_operations < N*log(N)` isn't going to work because it's actually `k*N*log(N)`, where `k` is an arbitrary constant;
> > - the "proper" way would be to test with increasingly large inputs and applying some math to calculate the growth factor, but that seems like a sizable project in itself, even assuming it's worth the implementation effort.
> > 
> Here http://eel.is/c++draft/stable.sort#5 lists the exact number of comparisons to do and that the number of projections is double of that. So I think we can use that value as an upper bound, for example with N = 10.
I'd rather do this in a follow-up. I think perhaps it would be good to have a dedicated test file for this, similar to the `robust_against_copying_*`.

================
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`.
+
----------------
Mordante wrote:
> var-const wrote:
> > Mordante wrote:
> > > Interesting since it's possible to allocate memory in `constexpr` functions.
> > I know that [[ http://wg21.link/p0202 | P0202 ]] was adding `constexpr` to algorithms. At the time (in 2017), making `stable_sort` `constexpr` was infeasible:
> > 
> > > Algorithms `stable_partition`, `inplace_merge` and `stable_sort` allocate memory, construct variables using placement new, use `unique_ptr` and do other things not acceptable in constexpr expressions. Making those algorithms constexpr seems to be a hard task that would require a lot of intrinsics. Those algorithms are not marked with constexpr in this wording.
> > 
> > I don't know if there's been any work in that direction since then. Perhaps it's ripe for a new paper?
> I didn't investigate the reason, but this is indeed what I expected :-)
> 
> Based on the unanimous consent in this poll https://github.com/cplusplus/papers/issues/1222#issuecomment-1159203566
> the time is indeed ripe and a paper is already available.
Thanks for finding that paper!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127834