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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/algorithm:5245
 
+template <class _Compare, class _RandomAccessIterator>
+_LIBCPP_CONSTEXPR_AFTER_CXX17 void
----------------
I don't think we need this declaration. Isn't it the same as the definition on line 5165?


================
Comment at: libcxx/include/algorithm:5502
+void
+sort(__wrap_iter<_Tp*> __first, __wrap_iter<_Tp*> __last, _Compare __comp)
+{
----------------
In a second step, couldn't we remove this overload to replace it by calls to `__unwrap_iter`?


================
Comment at: libcxx/include/algorithm:5364
+    }
+}
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93661