From b32e1b63eef7b6aa30d81f4b959ea519de2c4111 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 28 Jun 2024 15:22:33 +0100 Subject: [PATCH 1/3] gqsort: Add g_sort_array() and deprecate g_qsort_with_data() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The latter only accepts a `gint` as the number of elements in the array, which means that its use in `GArray` (and related array implementations) truncates at least half the potential array size. So, introduce a replacement for it which uses `size_t` for the number of elements. This is inline with what `qsort()` (or `qsort_r()`) actually does. Unfortunately we can’t directly use `qsort_r()` because it’s not guaranteed to be a stable sort. This fixes some `-Wsign-conversion` warnings (when building GLib with that enabled). Signed-off-by: Philip Withnall Helps: #3405 --- glib/garray.c | 40 ++++++++++++++++++++-------------------- glib/gqsort.c | 36 ++++++++++++++++++++++++++++++++---- glib/gqsort.h | 9 ++++++++- glib/tests/sort.c | 31 +++++++++++++++++++++++++++---- gobject/gvaluearray.c | 8 ++++---- 5 files changed, 91 insertions(+), 33 deletions(-) diff --git a/glib/garray.c b/glib/garray.c index e586bb5fa..413ea8539 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -925,11 +925,11 @@ g_array_sort (GArray *farray, /* Don't use qsort as we want a guaranteed stable sort */ if (array->len > 0) - g_qsort_with_data (array->data, - array->len, - array->elt_size, - (GCompareDataFunc)compare_func, - NULL); + g_sort_array (array->data, + array->len, + array->elt_size, + (GCompareDataFunc) compare_func, + NULL); } /** @@ -957,11 +957,11 @@ g_array_sort_with_data (GArray *farray, g_return_if_fail (array != NULL); if (array->len > 0) - g_qsort_with_data (array->data, - array->len, - array->elt_size, - compare_func, - user_data); + g_sort_array (array->data, + array->len, + array->elt_size, + compare_func, + user_data); } /** @@ -2413,11 +2413,11 @@ g_ptr_array_sort (GPtrArray *array, /* Don't use qsort as we want a guaranteed stable sort */ if (array->len > 0) - g_qsort_with_data (array->pdata, - array->len, - sizeof (gpointer), - (GCompareDataFunc)compare_func, - NULL); + g_sort_array (array->pdata, + array->len, + sizeof (gpointer), + (GCompareDataFunc) compare_func, + NULL); } /* Please keep this doc-comment in sync with @@ -2493,11 +2493,11 @@ g_ptr_array_sort_with_data (GPtrArray *array, g_return_if_fail (array != NULL); if (array->len > 0) - g_qsort_with_data (array->pdata, - array->len, - sizeof (gpointer), - compare_func, - user_data); + g_sort_array (array->pdata, + array->len, + sizeof (gpointer), + compare_func, + user_data); } static inline gint diff --git a/glib/gqsort.c b/glib/gqsort.c index 9d6bb1e14..657d94876 100644 --- a/glib/gqsort.c +++ b/glib/gqsort.c @@ -289,10 +289,14 @@ msort_r (void *b, size_t n, size_t s, GCompareDataFunc cmp, void *arg) * @compare_func: (scope call): function to compare elements * @user_data: data to pass to @compare_func * - * This is just like the standard C qsort() function, but - * the comparison routine accepts a user data argument. + * This is just like the standard C [`qsort()`](man:qsort(3)) function, but + * the comparison routine accepts a user data argument + * (like [`qsort_r()`](man:qsort_r(3))). * - * This is guaranteed to be a stable sort since version 2.32. + * Unlike `qsort()`, this is guaranteed to be a stable sort (since GLib 2.32). + * + * Deprecated: 2.82: `total_elems` is too small to represent larger arrays; use + * [func@GLib.sort_array] instead */ void g_qsort_with_data (gconstpointer pbase, @@ -301,5 +305,29 @@ g_qsort_with_data (gconstpointer pbase, GCompareDataFunc compare_func, gpointer user_data) { - msort_r ((gpointer)pbase, total_elems, size, compare_func, user_data); + g_sort_array (pbase, total_elems, size, compare_func, user_data); +} + +/** + * g_sort_array: + * @array: (not nullable) (array length=n_elements): start of array to sort + * @n_elements: number of elements in the array + * @element_size: size of each element + * @compare_func: (scope call): function to compare elements + * @user_data: data to pass to @compare_func + * + * This is just like the standard C [`qsort()`](man:qsort(3)) function, but + * the comparison routine accepts a user data argument + * (like [`qsort_r()`](man:qsort_r(3))). + * + * Unlike `qsort()`, this is guaranteed to be a stable sort. + */ +void +g_sort_array (const void *array, + size_t n_elements, + size_t element_size, + GCompareDataFunc compare_func, + void *user_data) +{ + msort_r ((void *) array, n_elements, element_size, compare_func, user_data); } diff --git a/glib/gqsort.h b/glib/gqsort.h index c04c0381e..611135f32 100644 --- a/glib/gqsort.h +++ b/glib/gqsort.h @@ -35,13 +35,20 @@ G_BEGIN_DECLS -GLIB_AVAILABLE_IN_ALL +GLIB_DEPRECATED_IN_2_82_FOR(g_sort_array) void g_qsort_with_data (gconstpointer pbase, gint total_elems, gsize size, GCompareDataFunc compare_func, gpointer user_data); +GLIB_AVAILABLE_IN_2_82 +void g_sort_array (const void *array, + size_t n_elements, + size_t element_size, + GCompareDataFunc compare_func, + void *user_data); + G_END_DECLS #endif /* __G_QSORT_H__ */ diff --git a/glib/tests/sort.c b/glib/tests/sort.c index e42e645d7..f251cb759 100644 --- a/glib/tests/sort.c +++ b/glib/tests/sort.c @@ -40,7 +40,7 @@ test_sort_basic (void) data[i] = g_random_int_range (0, 10000); } - g_qsort_with_data (data, 10000, sizeof (int), int_compare_data, NULL); + g_sort_array (data, 10000, sizeof (int), int_compare_data, NULL); for (i = 1; i < 10000; i++) g_assert_cmpint (data[i -1], <=, data[i]); @@ -63,7 +63,7 @@ test_sort_zero_elements (void) } /* 0 elements is a valid case */ - g_qsort_with_data (data, 0, sizeof (int), int_compare_data, NULL); + g_sort_array (data, 0, sizeof (int), int_compare_data, NULL); for (i = 0; i < 100; i++) g_assert_cmpint (data[i], ==, data_copy[i]); @@ -105,7 +105,7 @@ test_sort_stable (void) data[i].i = i; } - g_qsort_with_data (data, 10000, sizeof (SortItem), item_compare_data, NULL); + g_sort_array (data, 10000, sizeof (SortItem), item_compare_data, NULL); for (i = 1; i < 10000; i++) { @@ -129,7 +129,7 @@ test_sort_big (void) data[i].i = i; } - g_qsort_with_data (data, 10000, sizeof (BigItem), item_compare_data, NULL); + g_sort_array (data, 10000, sizeof (BigItem), item_compare_data, NULL); for (i = 1; i < 10000; i++) { @@ -140,6 +140,28 @@ test_sort_big (void) g_free (data); } +static void +test_sort_deprecated (void) +{ + gint *data; + gint i; + + data = g_malloc (10000 * sizeof (int)); + for (i = 0; i < 10000; i++) + { + data[i] = g_random_int_range (0, 10000); + } + +G_GNUC_BEGIN_IGNORE_DEPRECATIONS + g_qsort_with_data (data, 10000, sizeof (int), int_compare_data, NULL); +G_GNUC_END_IGNORE_DEPRECATIONS + + for (i = 1; i < 10000; i++) + g_assert_cmpint (data[i -1], <=, data[i]); + + g_free (data); +} + int main (int argc, char *argv[]) { @@ -149,6 +171,7 @@ main (int argc, char *argv[]) g_test_add_func ("/sort/zero-elements", test_sort_zero_elements); g_test_add_func ("/sort/stable", test_sort_stable); g_test_add_func ("/sort/big", test_sort_big); + g_test_add_func ("/sort/deprecated", test_sort_deprecated); return g_test_run (); } diff --git a/gobject/gvaluearray.c b/gobject/gvaluearray.c index 378b0a314..8c6b81d2b 100644 --- a/gobject/gvaluearray.c +++ b/gobject/gvaluearray.c @@ -364,9 +364,9 @@ g_value_array_sort_with_data (GValueArray *value_array, g_return_val_if_fail (compare_func != NULL, NULL); if (value_array->n_values) - g_qsort_with_data (value_array->values, - value_array->n_values, - sizeof (value_array->values[0]), - compare_func, user_data); + g_sort_array (value_array->values, + value_array->n_values, + sizeof (value_array->values[0]), + compare_func, user_data); return value_array; } From 7b435dfa7cfbfa9e3e77361f7b008d5b98b46b45 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 28 Jun 2024 15:25:30 +0100 Subject: [PATCH 2/3] garray: Fix g_ptr_array_insert() with indices > G_MAXINT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While an index greater than `G_MAXINT` can’t be passed to `g_ptr_array_insert()`, `-1` can be — and if that’s done with an array which has more than `G_MAXINT` elements in it, the new element will be inserted part-way through the array rather than being appended. Spotted by building with `-Wsign-conversion`. Signed-off-by: Philip Withnall Helps: #3405 --- glib/garray.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/glib/garray.c b/glib/garray.c index 413ea8539..f67099315 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -2338,23 +2338,23 @@ g_ptr_array_insert (GPtrArray *array, gpointer data) { GRealPtrArray *rarray = (GRealPtrArray *)array; + guint real_index; g_return_if_fail (rarray); g_return_if_fail (index_ >= -1); - g_return_if_fail (index_ <= (gint)rarray->len); + g_return_if_fail (index_ < 0 || (guint) index_ <= rarray->len); g_ptr_array_maybe_expand (rarray, 1u + rarray->null_terminated); - if (index_ < 0) - index_ = rarray->len; + real_index = (index_ >= 0) ? (guint) index_ : rarray->len; - if ((guint) index_ < rarray->len) - memmove (&(rarray->pdata[index_ + 1]), - &(rarray->pdata[index_]), - (rarray->len - index_) * sizeof (gpointer)); + if (real_index < rarray->len) + memmove (&(rarray->pdata[real_index + 1]), + &(rarray->pdata[real_index]), + (rarray->len - real_index) * sizeof (gpointer)); rarray->len++; - rarray->pdata[index_] = data; + rarray->pdata[real_index] = data; ptr_array_maybe_null_terminate (rarray); } From f953212cc5f5243e13006bc3e6fbd533a9b254e5 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 4 Jul 2024 12:34:20 +0100 Subject: [PATCH 3/3] tests: Add a test for g_value_array_sort_with_data() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s deprecated, but I was modifying it anyway and it didn’t have any coverage, so let’s add a simple test (as suggested by Michael Catanzaro). Signed-off-by: Philip Withnall --- gobject/tests/value.c | 47 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/gobject/tests/value.c b/gobject/tests/value.c index d1a90919a..4d5637880 100644 --- a/gobject/tests/value.c +++ b/gobject/tests/value.c @@ -664,6 +664,52 @@ test_valuearray_basic (void) g_value_array_free (a2); } +static gint +cmpint_with_data (gconstpointer a, + gconstpointer b, + gpointer user_data) +{ + const GValue *aa = a; + const GValue *bb = b; + + g_assert_cmpuint (GPOINTER_TO_UINT (user_data), ==, 123); + + return g_value_get_int (aa) - g_value_get_int (bb); +} + +static void +test_value_array_sort_with_data (void) +{ + GValueArray *a, *a2; + GValue v = G_VALUE_INIT; + + a = g_value_array_new (20); + + /* Try sorting an empty array. */ + a2 = g_value_array_sort_with_data (a, cmpint_with_data, GUINT_TO_POINTER (456)); + g_assert_cmpuint (a->n_values, ==, 0); + g_assert_true (a2 == a); + + /* Add some values and try sorting them. */ + g_value_init (&v, G_TYPE_INT); + for (unsigned int i = 0; i < 100; i++) + { + g_value_set_int (&v, 100 - i); + g_value_array_append (a, &v); + } + + g_assert_cmpint (a->n_values, ==, 100); + + a2 = g_value_array_sort_with_data (a, cmpint_with_data, GUINT_TO_POINTER (123)); + + for (unsigned int i = 0; i < a->n_values - 1; i++) + g_assert_cmpint (g_value_get_int (&a->values[i]), <=, g_value_get_int (&a->values[i+1])); + + g_assert_true (a2 == a); + + g_value_array_free (a); +} + /* We create some dummy objects with this relationship: * * GObject TestInterface @@ -765,6 +811,7 @@ main (int argc, char *argv[]) g_test_add_func ("/value/basic", test_value_basic); g_test_add_func ("/value/array/basic", test_valuearray_basic); + g_test_add_func ("/value/array/sort-with-data", test_value_array_sort_with_data); g_test_add_func ("/value/collection", test_collection); g_test_add_func ("/value/copying", test_copying); g_test_add_func ("/value/enum-transformation", test_enum_transformation);