[libc-commits] [libc] [llvm] [libc] Improve qsort (PR #120450)

[Lukas Bergdoll via libc-commits]

================
@@ -27,11 +31,51 @@
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
+template <bool USE_QUICKSORT, typename F>
+void unstable_sort_impl(void *array, size_t array_len, size_t elem_size,
+                        const F &is_less) {
+  if (array == nullptr || array_len == 0 || elem_size == 0)
+    return;
+
+  if constexpr (USE_QUICKSORT) {
+    switch (elem_size) {
+    case 4: {
+      auto arr_fixed_size = internal::ArrayFixedSize<4>(array, array_len);
+      quick_sort(arr_fixed_size, is_less);
+      return;
+    }
+    case 8: {
+      auto arr_fixed_size = internal::ArrayFixedSize<8>(array, array_len);
+      quick_sort(arr_fixed_size, is_less);
+      return;
+    }
+    case 16: {
+      auto arr_fixed_size = internal::ArrayFixedSize<16>(array, array_len);
+      quick_sort(arr_fixed_size, is_less);
+      return;
+    }
+    default:
+      auto arr_generic_size =
+          internal::ArrayGenericSize(array, array_len, elem_size);
+      quick_sort(arr_generic_size, is_less);
+      return;
+    }
+  } else {
+    auto arr_generic_size =
+        internal::ArrayGenericSize(array, array_len, elem_size);
+    heap_sort(arr_generic_size, is_less);
+  }
+}
+
+template <typename F>
+void unstable_sort(void *array, size_t array_len, size_t elem_size,
+                   const F &is_less) {
 #if LIBC_QSORT_IMPL == LIBC_QSORT_QUICK_SORT
-constexpr auto sort = quick_sort;
+  unstable_sort_impl<true, F>(array, array_len, elem_size, is_less);
 #elif LIBC_QSORT_IMPL == LIBC_QSORT_HEAP_SORT
-constexpr auto sort = heap_sort;
+  unstable_sort_impl<false, F>(array, array_len, elem_size, is_less);
 #endif
----------------
Voultapher wrote:

@michaelrj-google in my experience future proofing code for some vague future that is not concretely planned is a recipe for unnecessary complexity. Specifically here, if at some future point more than these two options will be added the `bool` is changed to an `enum` and problem solved. That said I doubt if adding a mergesort here is a good idea, since the current implementation is in-place and suddenly allocating large buffers in a function that previously didn't is recipe for unhappy users.

I think we need the impl function so we can test the code users will use, more on that in the other test comment.

https://github.com/llvm/llvm-project/pull/120450