[libcxx-commits] [PATCH] D93661: [libc++] [P0879] constexpr std::sort, and add new tests for it
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Dec 21 13:07:50 PST 2020
Quuxplusone added inline comments.
Comment at: libcxx/include/algorithm:3953
- // _Compare is known to be a reference type
typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
This comment was never correct. `_Compare` may be `Pred&`, or it may be `__debug_less<Pred>` without the `&`.
Comment at: libcxx/include/algorithm:4147
+ __less<uintptr_t> __comp;
+ _VSTD::__sort<__less<uintptr_t>&, uintptr_t*>((uintptr_t*)__first, (uintptr_t*)__last, __comp);
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`.
Comment at: libcxx/include/algorithm: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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits