This completes libc++'s implementation of P0879 "Constexpr for swap and swap related functions."
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0879r0.html
For the feature-macro adjustment, see
https://cplusplus.github.io/LWG/issue3256
Details
Details
- Reviewers
ldionne jdoerfert - Group Reviewers
Restricted Project - Commits
- rG493f1407927c: [libc++] [P0879] constexpr std::sort
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/algorithm | ||
|---|---|---|
| 3953 | This comment was never correct. _Compare may be Pred&, or it may be __debug_less<Pred> without the &. | |
| 4147 | I'm preserving the old code's strategy here. As I say in D92190, I'd like to change this to call _VSTD::sort(first, last, __less<void*>()) and explicitly instantiate __sort<,__less<void*>&, void*> in "libc++.a"; but I'd like to do that in a separate PR because it has ABI implications. I also note in passing that it would be awesome if we had a way to detect contiguous iterators in C++17-and-earlier, so that we could use that, instead of special-casing T** and __wrap_iter<T*>. This is related to the use of __unwrap_iter in std::copy; whatever we do should be applied consistently in both sort and copy. | |
| 5364 | 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. | |
Comment Actions
What's the status of this? Can you please rebase so CI runs (there were some CI issues during the holidays, I remember patch application failing). Ping me when this is ready for review, until then I'm marking as "changes requested".
| libcxx/include/algorithm | ||
|---|---|---|
| 5245 | I don't think we need this declaration. Isn't it the same as the definition on line 5165? | |
| 5364 | If we get rid of the __unwrap_iter overload for sort(), we can simply remove __sort_constexpr and that solves the question. WDYT? | |
| 5502 | In a second step, couldn't we remove this overload to replace it by calls to __unwrap_iter? | |
| libcxx/include/algorithm | ||
|---|---|---|
| 5502 | As of current main, no, because __unwrap_iter isn't optimized for non-trivially-copyable element types (it unwraps __wrap_iter<int*> but not __wrap_iter<string*>). If we land D94807 first, then yes. I suppose this implies that I should make D94807 a parent of this one and prioritize it. I think I agree that that would be a much nicer overall solution than the current soup of overload sets. | |
Comment Actions
Rebase on main (after landing D94807)
At least on OSX, this patch has no effect on the output of nm libc++.dylib or ls -l libc++.dylib. The effect on libc++.a is that it shrinks from 7968'560 to 7968'408 bytes; the diff in nm libc++.a | sort is this, which I don't see why this should happen at all, but anyway it's not a bad thing:
000000000001bfc0 T __ZNSt3__16locale5__impC2ERKS1_RKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEi 000000000001c6c0 T __ZNSt3__16locale5__impC1ERKS1_RKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEi 000000000001c6d0 T __ZNSt3__16locale5__impC2ERKS1_S3_i -000000000001d188 b __ZZNSt3__117iostream_categoryEvE1s -000000000001d190 b __ZGVZNSt3__117iostream_categoryEvE1s -000000000001d198 b __ZZNSt3__18ios_base5iwordEiE5error -000000000001d1a0 b __ZZNSt3__18ios_base5pwordEiE5error -000000000001d1a8 S __ZNSt3__18ios_base9__xindex_E +000000000001d180 b __ZZNSt3__117iostream_categoryEvE1s +000000000001d188 b __ZGVZNSt3__117iostream_categoryEvE1s +000000000001d190 b __ZZNSt3__18ios_base5iwordEiE5error +000000000001d198 b __ZZNSt3__18ios_base5pwordEiE5error +000000000001d1a0 S __ZNSt3__18ios_base9__xindex_E 000000000001d4e8 b __ZZNSt3__115future_categoryEvE3__f 000000000001d4f0 b __ZGVZNSt3__115future_categoryEvE3__f 000000000001e430 T __ZNSt3__16locale5__impC1ERKS1_S3_i 000000000001e440 T __ZNSt3__16locale5__impC2ERKS1_PNS0_5facetEl
| libcxx/include/algorithm | ||
|---|---|---|
| 5364 | 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 | |
| libcxx/include/algorithm | ||
|---|---|---|
| 5364 |
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]. | |
| libcxx/include/algorithm | ||
|---|---|---|
| 5364 | 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? | |
This comment was never correct. _Compare may be Pred&, or it may be __debug_less<Pred> without the &.