[libcxx-commits] [PATCH] D93661: [libc++] [P0879] constexpr std::sort, and add new tests for it

[Danila Kutenin via Phabricator via libcxx-commits]

danlark added inline comments.


================
Comment at: libcxx/include/algorithm:5364
+    }
+}
+
----------------
ldionne wrote:
> Quuxplusone wrote:
> > In the `nth_element` review (D93557) I decided to constexpr-ify the algorithm itself. In this one, I decided that the simplest course of action would be to switch on `is_constant_evaluated` and if we're doing it at compile-time let's just do a plain old heapsort because that's already constexpr-friendly, and then we don't have to worry about making `__sort` itself constexpr.
> > 
> > So the name here might be misleading, and I'd take suggestions for better names. Here `__sort_constexpr` means "This algorithm is guaranteed constexpr-friendly," but it's //above// the switch-dispatch-thingie, whereas in e.g. `__copy_constexpr` it's //below// the switch-dispatch-thingie.
> > 
> > I don't want to change the name of `__sort` itself in this specific patch, because that would have ABI implications (some explicit template instantiations are placed in "libc++.a"). I'm excited for the prospect of ABI breakage ;) but I'd like to confine any ABI breakage to a separate PR.
> If we get rid of the `__unwrap_iter` overload for `sort()`, we can simply remove `__sort_constexpr` and that solves the question. WDYT?
One thing to think about:

Yet, std::sort does not guarantee the order of equal elements. With the current patch it may happen that constexpr std::sort and usual std::sort return different ranges because of different stability guarantees and that might be not user friendly, i.e. the property of determinism is lost.

I don't have a strong opinion on that question, however, I'd prefer constexprify this function instead of calling partial_sort


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93661