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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

In D127834#3595819 <https://reviews.llvm.org/D127834#3595819>, @philnik wrote:

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

LGTM, I leave the final approval to @philnik.



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.sort/stable.sort/ranges.stable.sort.pass.cpp:246
+  }
+
+  { // `std::ranges::dangling` is returned.
----------------
var-const wrote:
> 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_*`.
Sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127834