[libc-commits] [libc] [libc] Refactor qsort code (PR #198781)
via libc-commits
libc-commits at lists.llvm.org
Wed May 20 06:32:58 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libc
Author: Victor Campos (vhscampos)
<details>
<summary>Changes</summary>
This patch makes the following changes:
- Refactor the internal sorting functions to reduce code duplication.
- Move the testing machinery done for the testing of `qsort_r` to a shared place.
These changes are done in anticipation to the introduction of Annex K's `qsort_s`. This function shares most of its semantics with `qsort_r`, therefore most of the testing logic can be shared between the two. Besides, `qsort`, `qsort_r` and `qsort_r` are all very similar, hence we can attempt to reduce duplication a bit more.
---
Full diff: https://github.com/llvm/llvm-project/pull/198781.diff
6 Files Affected:
- (modified) libc/src/stdlib/qsort.cpp (+1-5)
- (modified) libc/src/stdlib/qsort_r.cpp (+1-6)
- (modified) libc/src/stdlib/qsort_util.h (+22-3)
- (modified) libc/test/src/stdlib/CMakeLists.txt (+2-1)
- (added) libc/test/src/stdlib/QsortReentrantTest.h (+150)
- (modified) libc/test/src/stdlib/qsort_r_test.cpp (+2-134)
``````````diff
diff --git a/libc/src/stdlib/qsort.cpp b/libc/src/stdlib/qsort.cpp
index f66b686d4e54b..46a74fb9118a3 100644
--- a/libc/src/stdlib/qsort.cpp
+++ b/libc/src/stdlib/qsort.cpp
@@ -18,11 +18,7 @@ LLVM_LIBC_FUNCTION(void, qsort,
(void *array, size_t array_size, size_t elem_size,
int (*compare)(const void *, const void *))) {
- 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);
+ internal::unstable_sort(array, array_size, elem_size, compare);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdlib/qsort_r.cpp b/libc/src/stdlib/qsort_r.cpp
index 47448201eddbd..65afcee77885d 100644
--- a/libc/src/stdlib/qsort_r.cpp
+++ b/libc/src/stdlib/qsort_r.cpp
@@ -18,12 +18,7 @@ LLVM_LIBC_FUNCTION(void, qsort_r,
(void *array, size_t array_size, size_t elem_size,
int (*compare)(const void *, const void *, void *),
void *arg)) {
-
- 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);
+ internal::unstable_sort(array, array_size, elem_size, compare, arg);
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdlib/qsort_util.h b/libc/src/stdlib/qsort_util.h
index 7882b829d3274..24f41907d78c5 100644
--- a/libc/src/stdlib/qsort_util.h
+++ b/libc/src/stdlib/qsort_util.h
@@ -64,10 +64,29 @@ LIBC_INLINE void unstable_sort_impl(void *array, size_t array_len,
}
template <typename F>
+LIBC_INLINE void unstable_sort_dispatch(void *array, size_t array_len,
+ size_t elem_size, F is_less) {
+ constexpr bool use_quick_sort = (LIBC_QSORT_IMPL == LIBC_QSORT_QUICK_SORT);
+ unstable_sort_impl<use_quick_sort>(array, array_len, elem_size, is_less);
+}
+
+template <typename CmpFn>
+LIBC_INLINE void unstable_sort(void *array, size_t array_len, size_t elem_size,
+ CmpFn compare) {
+ const auto is_less = [compare](const void *a, const void *b) -> bool {
+ return compare(a, b) < 0;
+ };
+ unstable_sort_dispatch(array, array_len, elem_size, is_less);
+}
+
+template <typename CmpFn>
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);
+ CmpFn compare, void *context) {
+ const auto is_less = [compare, context](const void *a,
+ const void *b) -> bool {
+ return compare(a, b, context) < 0;
+ };
+ unstable_sort_dispatch(array, array_len, elem_size, is_less);
}
} // namespace internal
diff --git a/libc/test/src/stdlib/CMakeLists.txt b/libc/test/src/stdlib/CMakeLists.txt
index a7b7c9269fcee..270a7a2291293 100644
--- a/libc/test/src/stdlib/CMakeLists.txt
+++ b/libc/test/src/stdlib/CMakeLists.txt
@@ -360,8 +360,9 @@ add_libc_test(
libc-stdlib-tests
SRCS
qsort_r_test.cpp
+ HDRS
+ QsortReentrantTest.h
DEPENDS
- libc.hdr.types.size_t
libc.src.stdlib.qsort_r
)
diff --git a/libc/test/src/stdlib/QsortReentrantTest.h b/libc/test/src/stdlib/QsortReentrantTest.h
new file mode 100644
index 0000000000000..4453a27b01f38
--- /dev/null
+++ b/libc/test/src/stdlib/QsortReentrantTest.h
@@ -0,0 +1,150 @@
+
+//===-- A template class for testing reentrant qsort functions --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "test/UnitTest/Test.h"
+
+template <typename QsortFnTy, typename SizeTy>
+class QsortReentrantTest : public LIBC_NAMESPACE::testing::Test {
+private:
+ static int int_compare_count(const void *l, const void *r, void *count_arg) {
+ int li = *static_cast<const int *>(l);
+ int ri = *static_cast<const int *>(r);
+ SizeTy *count = static_cast<SizeTy *>(count_arg);
+ *count = *count + 1;
+ if (li == ri)
+ return 0;
+ else if (li > ri)
+ return 1;
+ else
+ return -1;
+ }
+
+ struct PriorityVal {
+ int priority;
+ int size;
+ };
+
+ static int compare_priority_val(const PriorityVal *l, const PriorityVal *r) {
+ // Subtracting the priorities is unsafe, but it's fine for this test.
+ int priority_diff = l->priority - r->priority;
+ if (priority_diff != 0) {
+ return priority_diff;
+ }
+ if (l->size == r->size) {
+ return 0;
+ } else if (l->size > r->size) {
+ return 1;
+ } else {
+ return -1;
+ }
+ }
+
+ // The following test is intended to mimic the CPP library pattern of having a
+ // comparison function that takes a specific type, which is passed to a
+ // library that then needs to sort an array of that type. The library can't
+ // safely pass the comparison function to qsort because a function that takes
+ // const T* being cast to a function that takes const void* is undefined
+ // behavior. The safer pattern is to pass a type erased comparator that calls
+ // into the typed comparator to qsort_r.
+ template <typename T>
+ static int type_erased_comp(const void *l, const void *r,
+ void *erased_func_ptr) {
+ typedef int (*TypedComp)(const T *, const T *);
+ TypedComp typed_func_ptr = reinterpret_cast<TypedComp>(erased_func_ptr);
+ const T *lt = static_cast<const T *>(l);
+ const T *rt = static_cast<const T *>(r);
+ return typed_func_ptr(lt, rt);
+ }
+
+public:
+ void sorted_array(QsortFnTy func) {
+ int array[25] = {10, 23, 33, 35, 55, 70, 71, 100, 110,
+ 123, 133, 135, 155, 170, 171, 1100, 1110, 1123,
+ 1133, 1135, 1155, 1170, 1171, 11100, 12310};
+ constexpr SizeTy ARRAY_SIZE = sizeof(array) / sizeof(int);
+
+ SizeTy count = 0;
+
+ func(array, ARRAY_SIZE, sizeof(int), int_compare_count, &count);
+
+ ASSERT_LE(array[0], 10);
+ ASSERT_LE(array[1], 23);
+ ASSERT_LE(array[2], 33);
+ ASSERT_LE(array[3], 35);
+ ASSERT_LE(array[4], 55);
+ ASSERT_LE(array[5], 70);
+ ASSERT_LE(array[6], 71);
+ ASSERT_LE(array[7], 100);
+ ASSERT_LE(array[8], 110);
+ ASSERT_LE(array[9], 123);
+ ASSERT_LE(array[10], 133);
+ ASSERT_LE(array[11], 135);
+ ASSERT_LE(array[12], 155);
+ ASSERT_LE(array[13], 170);
+ ASSERT_LE(array[14], 171);
+ ASSERT_LE(array[15], 1100);
+ ASSERT_LE(array[16], 1110);
+ ASSERT_LE(array[17], 1123);
+ ASSERT_LE(array[18], 1133);
+ ASSERT_LE(array[19], 1135);
+ ASSERT_LE(array[20], 1155);
+ ASSERT_LE(array[21], 1170);
+ ASSERT_LE(array[22], 1171);
+ ASSERT_LE(array[23], 11100);
+ ASSERT_LE(array[24], 12310);
+
+ // This is a sorted list, but there still have to have been at least N - 1
+ // comparisons made.
+ ASSERT_GE(count, ARRAY_SIZE - 1);
+ }
+
+ void reverse_sorted_array(QsortFnTy func) {
+ int array[25] = {25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13,
+ 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1};
+ constexpr SizeTy ARRAY_SIZE = sizeof(array) / sizeof(int);
+
+ SizeTy count = 0;
+
+ func(array, ARRAY_SIZE, sizeof(int), int_compare_count, &count);
+
+ for (int i = 0; i < int(ARRAY_SIZE - 1); ++i)
+ ASSERT_LE(array[i], i + 1);
+
+ ASSERT_GE(count, ARRAY_SIZE);
+ }
+
+ void safe_type_erasure(QsortFnTy func) {
+ PriorityVal array[5] = {
+ {10, 3}, {1, 10}, {-1, 100}, {10, 0}, {3, 3},
+ };
+ constexpr SizeTy ARRAY_SIZE = sizeof(array) / sizeof(PriorityVal);
+
+ func(array, ARRAY_SIZE, sizeof(PriorityVal), type_erased_comp<PriorityVal>,
+ reinterpret_cast<void *>(compare_priority_val));
+
+ EXPECT_EQ(array[0].priority, -1);
+ EXPECT_EQ(array[0].size, 100);
+ EXPECT_EQ(array[1].priority, 1);
+ EXPECT_EQ(array[1].size, 10);
+ EXPECT_EQ(array[2].priority, 3);
+ EXPECT_EQ(array[2].size, 3);
+ EXPECT_EQ(array[3].priority, 10);
+ EXPECT_EQ(array[3].size, 0);
+ EXPECT_EQ(array[4].priority, 10);
+ EXPECT_EQ(array[4].size, 3);
+ }
+};
+
+#define QSORTREENTRANT_TEST(name, func, sizetype) \
+ using LlvmLibc##name##Test = QsortReentrantTest<decltype(func), sizetype>; \
+ TEST_F(LlvmLibc##name##Test, SortedArray) { sorted_array(func); } \
+ TEST_F(LlvmLibc##name##Test, ReverseSortedArray) { \
+ reverse_sorted_array(func); \
+ } \
+ TEST_F(LlvmLibc##name##Test, SafeTypeErasure) { safe_type_erasure(func); }
diff --git a/libc/test/src/stdlib/qsort_r_test.cpp b/libc/test/src/stdlib/qsort_r_test.cpp
index f18923618ed5e..b1fff5a1bc3dd 100644
--- a/libc/test/src/stdlib/qsort_r_test.cpp
+++ b/libc/test/src/stdlib/qsort_r_test.cpp
@@ -6,139 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "QsortReentrantTest.h"
#include "src/stdlib/qsort_r.h"
-#include "test/UnitTest/Test.h"
-
-#include "hdr/types/size_t.h"
-
-static int int_compare_count(const void *l, const void *r, void *count_arg) {
- int li = *reinterpret_cast<const int *>(l);
- int ri = *reinterpret_cast<const int *>(r);
- size_t *count = reinterpret_cast<size_t *>(count_arg);
- *count = *count + 1;
- if (li == ri)
- return 0;
- else if (li > ri)
- return 1;
- else
- return -1;
-}
-
-TEST(LlvmLibcQsortRTest, SortedArray) {
- int array[25] = {10, 23, 33, 35, 55, 70, 71, 100, 110,
- 123, 133, 135, 155, 170, 171, 1100, 1110, 1123,
- 1133, 1135, 1155, 1170, 1171, 11100, 12310};
- constexpr size_t ARRAY_SIZE = sizeof(array) / sizeof(int);
-
- size_t count = 0;
-
- LIBC_NAMESPACE::qsort_r(array, ARRAY_SIZE, sizeof(int), int_compare_count,
- &count);
-
- ASSERT_LE(array[0], 10);
- ASSERT_LE(array[1], 23);
- ASSERT_LE(array[2], 33);
- ASSERT_LE(array[3], 35);
- ASSERT_LE(array[4], 55);
- ASSERT_LE(array[5], 70);
- ASSERT_LE(array[6], 71);
- ASSERT_LE(array[7], 100);
- ASSERT_LE(array[8], 110);
- ASSERT_LE(array[9], 123);
- ASSERT_LE(array[10], 133);
- ASSERT_LE(array[11], 135);
- ASSERT_LE(array[12], 155);
- ASSERT_LE(array[13], 170);
- ASSERT_LE(array[14], 171);
- ASSERT_LE(array[15], 1100);
- ASSERT_LE(array[16], 1110);
- ASSERT_LE(array[17], 1123);
- ASSERT_LE(array[18], 1133);
- ASSERT_LE(array[19], 1135);
- ASSERT_LE(array[20], 1155);
- ASSERT_LE(array[21], 1170);
- ASSERT_LE(array[22], 1171);
- ASSERT_LE(array[23], 11100);
- ASSERT_LE(array[24], 12310);
-
- // This is a sorted list, but there still have to have been at least N - 1
- // comparisons made.
- ASSERT_GE(count, ARRAY_SIZE - 1);
-}
-
-TEST(LlvmLibcQsortRTest, ReverseSortedArray) {
- int array[25] = {25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13,
- 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1};
- constexpr size_t ARRAY_SIZE = sizeof(array) / sizeof(int);
-
- size_t count = 0;
-
- LIBC_NAMESPACE::qsort_r(array, ARRAY_SIZE, sizeof(int), int_compare_count,
- &count);
-
- for (int i = 0; i < int(ARRAY_SIZE - 1); ++i)
- ASSERT_LE(array[i], i + 1);
-
- ASSERT_GE(count, ARRAY_SIZE);
-}
-
-// The following test is intended to mimic the CPP library pattern of having a
-// comparison function that takes a specific type, which is passed to a library
-// that then needs to sort an array of that type. The library can't safely pass
-// the comparison function to qsort because a function that takes const T*
-// being cast to a function that takes const void* is undefined behavior. The
-// safer pattern is to pass a type erased comparator that calls into the typed
-// comparator to qsort_r.
-
-struct PriorityVal {
- int priority;
- int size;
-};
-
-static int compare_priority_val(const PriorityVal *l, const PriorityVal *r) {
- // Subtracting the priorities is unsafe, but it's fine for this test.
- int priority_diff = l->priority - r->priority;
- if (priority_diff != 0) {
- return priority_diff;
- }
- if (l->size == r->size) {
- return 0;
- } else if (l->size > r->size) {
- return 1;
- } else {
- return -1;
- }
-}
-
-template <typename T>
-static int type_erased_comp(const void *l, const void *r,
- void *erased_func_ptr) {
- typedef int (*TypedComp)(const T *, const T *);
- TypedComp typed_func_ptr = reinterpret_cast<TypedComp>(erased_func_ptr);
- const T *lt = reinterpret_cast<const T *>(l);
- const T *rt = reinterpret_cast<const T *>(r);
- return typed_func_ptr(lt, rt);
-}
-
-TEST(LlvmLibcQsortRTest, SafeTypeErasure) {
- PriorityVal array[5] = {
- {10, 3}, {1, 10}, {-1, 100}, {10, 0}, {3, 3},
- };
- constexpr size_t ARRAY_SIZE = sizeof(array) / sizeof(PriorityVal);
-
- LIBC_NAMESPACE::qsort_r(array, ARRAY_SIZE, sizeof(PriorityVal),
- type_erased_comp<PriorityVal>,
- reinterpret_cast<void *>(compare_priority_val));
-
- EXPECT_EQ(array[0].priority, -1);
- EXPECT_EQ(array[0].size, 100);
- EXPECT_EQ(array[1].priority, 1);
- EXPECT_EQ(array[1].size, 10);
- EXPECT_EQ(array[2].priority, 3);
- EXPECT_EQ(array[2].size, 3);
- EXPECT_EQ(array[3].priority, 10);
- EXPECT_EQ(array[3].size, 0);
- EXPECT_EQ(array[4].priority, 10);
- EXPECT_EQ(array[4].size, 3);
-}
+QSORTREENTRANT_TEST(QsortR, LIBC_NAMESPACE::qsort_r, size_t)
``````````
</details>
https://github.com/llvm/llvm-project/pull/198781
More information about the libc-commits
mailing list