[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
+    }
+}
+
----------------
Quuxplusone wrote:
> danlark wrote:
> > 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
> > With the current patch it may happen that constexpr std::sort and usual std::sort return different ranges [...] i.e. the property of determinism is lost.
> 
> That's a good point I hadn't thought of: Maybe constexpr sort produces (1, 2a, 2c, 2b, 3) while runtime sort produces (1, 2b, 2a, 2c, 3).  However, arguably, libc++ doesn't want to be in the business of making guarantees about sort's behavior/stability —  the user should just avoid writing code that relies on those outcomes.
> 
> Vice versa, if we make the existing sort algorithm constexpr-friendly in order to make users happy, I think users would //even more so// want us to do it without altering any of its existing outcomes. They might object if libc++ v12.0 sort produces (1, 2b, 2a, 2c, 3) [at runtime only] while libc++ v13.0 sort produces (1, 2b, 2c, 2a, 3) [at runtime and compile-time].
Depending on the exact values is definitely the user's problem. I don't know if sorting algorithms must be deterministic from the input data but I definitely see cases where some users might expose constant array and check for sorting equality during the runtime

Many questions and unfortunately standard does not answer them fully AFAIK. The algorithm should definitely be deterministic but does changing the evaluation context counts as input?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93661