Apply review comments

Author: Voultapher

libc/src/stdlib/heap_sort.h

@@ -19,7 +19,7 @@ namespace internal {
 // Follow the implementation in https://en.wikipedia.org/wiki/Heapsort.
 
 template <typename A, typename F>
-void heap_sort(const A &array, const F &is_less) {
+LIBC_INLINE void heap_sort(const A &array, const F &is_less) {
   size_t end = array.len();
   size_t start = end / 2;
 

libc/src/stdlib/qsort.cpp

@@ -18,10 +18,12 @@ namespace LIBC_NAMESPACE_DECL {
 LLVM_LIBC_FUNCTION(void, qsort,
                    (void *array, size_t array_size, size_t elem_size,
                     int (*compare)(const void *, const void *))) {
-  internal::unstable_sort(array, array_size, elem_size,
-                          [compare](const void *a, const void *b) -> bool {
-                            return compare(a, b) < 0;
-                          });
+
+  const auto is_less = [compare](const void *a, const void *b) -> bool {
+    return compare(a, b) < 0;
+  };
+
+  internal::unstable_sort(array, array_size, elem_size, is_less);
 }
 
 } // namespace LIBC_NAMESPACE_DECL

libc/src/stdlib/qsort_data.h

@@ -27,15 +27,15 @@ class ArrayGenericSize {
   }
 
 public:
-  ArrayGenericSize(void *a, size_t s, size_t e)
+  LIBC_INLINE ArrayGenericSize(void *a, size_t s, size_t e)
       : array_base(reinterpret_cast<cpp::byte *>(a)), array_len(s),
         elem_size(e) {}
 
   static constexpr bool has_fixed_size() { return false; }
 
   LIBC_INLINE void *get(size_t i) const { return get_internal(i); }
 
-  void swap(size_t i, size_t j) const {
+  LIBC_INLINE void swap(size_t i, size_t j) const {
     // It's possible to use 8 byte blocks with `uint64_t`, but that
     // generates more machine code as the remainder loop gets
     // unrolled, plus 4 byte operations are more likely to be
@@ -97,7 +97,7 @@ template <size_t ELEM_SIZE> class ArrayFixedSize {
   }
 
 public:
-  ArrayFixedSize(void *a, size_t s)
+  LIBC_INLINE ArrayFixedSize(void *a, size_t s)
       : array_base(reinterpret_cast<cpp::byte *>(a)), array_len(s) {}
 
   // Beware this function is used a heuristic for cheap to swap types, so

libc/src/stdlib/qsort_pivot.h

@@ -73,10 +73,7 @@ size_t median3(const A &array, size_t a, size_t b, size_t c, const F &is_less) {
     // If x=y=1 then a < b, c. In this case we want to return min(b, c).
     // By toggling the outcome of b < c using XOR x we get this behavior.
     const bool z = is_less(b_ptr, c_ptr);
-    if (z ^ x)
-      return c;
-    else
-      return b;
+    return z ^ x ? c : b;
   } else {
     // Either c <= a < b or b <= a < c, thus a is our median.
     return a;

libc/src/stdlib/qsort_r.cpp

@@ -20,10 +20,11 @@ LLVM_LIBC_FUNCTION(void, qsort_r,
                     int (*compare)(const void *, const void *, void *),
                     void *arg)) {
 
-  internal::unstable_sort(array, array_size, elem_size,
-                          [compare, arg](const void *a, const void *b) -> bool {
-                            return compare(a, b, arg) < 0;
-                          });
+  const auto is_less = [compare, arg](const void *a, const void *b) -> bool {
+    return compare(a, b, arg) < 0;
+  };
+
+  internal::unstable_sort(array, array_size, elem_size, is_less);
 }
 
 } // namespace LIBC_NAMESPACE_DECL

libc/src/stdlib/qsort_util.h

@@ -32,8 +32,8 @@ 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) {
+LIBC_INLINE 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;
 
@@ -68,13 +68,10 @@ void unstable_sort_impl(void *array, size_t array_len, size_t elem_size,
 }
 
 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
-  unstable_sort_impl<true, F>(array, array_len, elem_size, is_less);
-#elif LIBC_QSORT_IMPL == LIBC_QSORT_HEAP_SORT
-  unstable_sort_impl<false, F>(array, array_len, elem_size, is_less);
-#endif
+LIBC_INLINE void unstable_sort(void *array, size_t array_len, size_t elem_size,
+                               const F &is_less) {
+#define USE_QUICK_SORT ((LIBC_QSORT_IMPL) == (LIBC_QSORT_QUICK_SORT))
+  unstable_sort_impl<USE_QUICK_SORT, F>(array, array_len, elem_size, is_less);
 }
 
 } // namespace internal

libc/src/stdlib/quick_sort.h

@@ -24,8 +24,9 @@ namespace internal {
 // https://github.com/Voultapher/sort-research-rs/blob/main/writeup/lomcyc_partition/text.md.
 // Simplified to avoid having to stack allocate.
 template <typename A, typename F>
-size_t partition_lomuto_branchless(const A &array, const void *pivot,
-                                   const F &is_less) {
+LIBC_INLINE size_t partition_lomuto_branchless(const A &array,
+                                               const void *pivot,
+                                               const F &is_less) {
   const size_t array_len = array.len();
 
   size_t left = 0;
@@ -49,8 +50,8 @@ size_t partition_lomuto_branchless(const A &array, const void *pivot,
 // cyclic permutation is to have more efficient swapping, but we don't
 // know the element size so this isn't applicable here either.
 template <typename A, typename F>
-size_t partition_hoare_branchy(const A &array, const void *pivot,
-                               const F &is_less) {
+LIBC_INLINE size_t partition_hoare_branchy(const A &array, const void *pivot,
+                                           const F &is_less) {
   const size_t array_len = array.len();
 
   size_t left = 0;
@@ -78,7 +79,8 @@ size_t partition_hoare_branchy(const A &array, const void *pivot,
 }
 
 template <typename A, typename F>
-size_t partition(const A &array, size_t pivot_index, const F &is_less) {
+LIBC_INLINE size_t partition(const A &array, size_t pivot_index,
+                             const F &is_less) {
   // Place the pivot at the beginning of the array.
   if (pivot_index != 0) {
     array.swap(0, pivot_index);
@@ -104,8 +106,8 @@ size_t partition(const A &array, size_t pivot_index, const F &is_less) {
 }
 
 template <typename A, typename F>
-void quick_sort_impl(A &array, const void *ancestor_pivot, size_t limit,
-                     const F &is_less) {
+LIBC_INLINE void quick_sort_impl(A &array, const void *ancestor_pivot,
+                                 size_t limit, const F &is_less) {
   while (true) {
     const size_t array_len = array.len();
     if (array_len <= 1)
@@ -167,7 +169,8 @@ void quick_sort_impl(A &array, const void *ancestor_pivot, size_t limit,
 
 constexpr size_t ilog2(size_t n) { return cpp::bit_width(n) - 1; }
 
-template <typename A, typename F> void quick_sort(A &array, const F &is_less) {
+template <typename A, typename F>
+LIBC_INLINE void quick_sort(A &array, const F &is_less) {
   const void *ancestor_pivot = nullptr;
   // Limit the number of imbalanced partitions to `2 * floor(log2(len))`.
   // The binary OR by one is used to eliminate the zero-check in the logarithm.

libc/test/src/stdlib/SortingTest.h

@@ -10,8 +10,6 @@
 #include "src/stdlib/qsort.h"
 #include "test/UnitTest/Test.h"
 
-#include <iostream>
-
 class SortingTest : public LIBC_NAMESPACE::testing::Test {
 
   using SortingRoutine = void (*)(void *array, size_t array_len,

libc/test/src/stdlib/heap_sort_test.cpp

@@ -14,11 +14,13 @@ void heap_sort(void *array, size_t array_size, size_t elem_size,
 
   constexpr bool USE_QUICKSORT = false;
 
+  const auto is_less = [compare](const void *a,
+                                 const void *b) noexcept -> bool {
+    return compare(a, b) < 0;
+  };
+
   LIBC_NAMESPACE::internal::unstable_sort_impl<USE_QUICKSORT>(
-      array, array_size, elem_size,
-      [compare](const void *a, const void *b) noexcept -> bool {
-        return compare(a, b) < 0;
-      });
+      array, array_size, elem_size, is_less);
 }
 
 LIST_SORTING_TESTS(HeapSort, heap_sort);

libc/test/src/stdlib/quick_sort_test.cpp

@@ -13,11 +13,13 @@ void quick_sort(void *array, size_t array_size, size_t elem_size,
                 int (*compare)(const void *, const void *)) {
   constexpr bool USE_QUICKSORT = true;
 
+  const auto is_less = [compare](const void *a,
+                                 const void *b) noexcept -> bool {
+    return compare(a, b) < 0;
+  };
+
   LIBC_NAMESPACE::internal::unstable_sort_impl<USE_QUICKSORT>(
-      array, array_size, elem_size,
-      [compare](const void *a, const void *b) noexcept -> bool {
-        return compare(a, b) < 0;
-      });
+      array, array_size, elem_size, is_less);
 }
 
 LIST_SORTING_TESTS(Qsort, quick_sort);