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

[Michael Jones 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
----------------
michaelrj-google wrote:

I'd argue that we don't need `unstable_sort_impl` at all. If you move the basic checks to the start of `unstable_sort` then it's not split by templating.
Also, we don't want to assume that there are only two sorting implementation. Currently there is quicksort and heapsort, but in future it may be useful to have others (e.g. mergesort) or even variants of the existing sorts. Github won't let me attach a suggestion to this comment so I've just copied the code here:
```
template <typename F>
void unstable_sort(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 LIBC_QSORT_IMPL == LIBC_QSORT_QUICK_SORT
  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;
    }
#elif LIBC_QSORT_IMPL == LIBC_QSORT_HEAP_SORT
  auto arr_generic_size =
      internal::ArrayGenericSize(array, array_len, elem_size);
  heap_sort(arr_generic_size, is_less);
#endif
}
```

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