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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93661