From 3eeec77800ab6019febe8fb73ec34533770752c5 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 9 May 2018 15:34:39 +0100 Subject: [PATCH 1/3] garray: Fix (nullable) annotation on GArray.[prepend|insert]_vals() They do both accept NULL value arrays, but only if the number of elements in the value array is zero. Fix the annotations and mention this in the documentation. Signed-off-by: Philip Withnall https://bugzilla.gnome.org/show_bug.cgi?id=795975 --- glib/garray.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/glib/garray.c b/glib/garray.c index 914eaee34..5207eccc3 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -439,11 +439,14 @@ g_array_append_vals (GArray *farray, /** * g_array_prepend_vals: * @array: a #GArray - * @data: (not nullable): a pointer to the elements to prepend to the start of the array - * @len: the number of elements to prepend + * @data: (nullable): a pointer to the elements to prepend to the start of the array + * @len: the number of elements to prepend, which may be zero * * Adds @len elements onto the start of the array. * + * @data may be %NULL if (and only if) @len is zero. If @len is zero, this + * function is a no-op. + * * This operation is slower than g_array_append_vals() since the * existing elements in the array have to be moved to make space for * the new elements. @@ -498,11 +501,14 @@ g_array_prepend_vals (GArray *farray, * g_array_insert_vals: * @array: a #GArray * @index_: the index to place the elements at - * @data: (not nullable): a pointer to the elements to insert + * @data: (nullable): a pointer to the elements to insert * @len: the number of elements to insert * * Inserts @len elements into a #GArray at the given index. * + * @data may be %NULL if (and only if) @len is zero. If @len is zero, this + * function is a no-op. + * * Returns: the #GArray */ /** From 89f45e96b246178534c6e0180fbf754596e934d9 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 9 May 2018 15:35:23 +0100 Subject: [PATCH 2/3] garray: Allow over-allocation in g_array_insert_vals() Previously, g_array_insert_vals() would crash if called with an index off the end of the array. This is inconsistent with the behaviour of other methods (like g_array_set_size()), which will expand the array as necessary. Modify g_array_insert_vals() to expand the array as necessary. New array elements will be cleared to zero if the GArray was constructed with (clear_ == TRUE). Signed-off-by: Philip Withnall https://bugzilla.gnome.org/show_bug.cgi?id=795975 --- glib/garray.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/glib/garray.c b/glib/garray.c index 5207eccc3..5fa8c4663 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -506,6 +506,11 @@ g_array_prepend_vals (GArray *farray, * * Inserts @len elements into a #GArray at the given index. * + * If @index_ is greater than the array’s current length, the array is expanded. + * The elements between the old end of the array and the newly inserted elements + * will be initialised to zero if the array was configured to clear elements; + * otherwise their values will be undefined. + * * @data may be %NULL if (and only if) @len is zero. If @len is zero, this * function is a no-op. * @@ -538,6 +543,11 @@ g_array_insert_vals (GArray *farray, if (len == 0) return farray; + /* Is the index off the end of the array, and hence do we need to over-allocate + * and clear some elements? */ + if (index_ >= array->len) + return g_array_append_vals (g_array_set_size (farray, index_), data, len); + g_array_maybe_expand (array, len); memmove (g_array_elt_pos (array, len + index_), From 80243e65b6d28b3338725be1b2ec69f97a7f9781 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 9 May 2018 15:37:01 +0100 Subject: [PATCH 3/3] tests: Expand GArray test coverage to cover all construction forms Previously, there was very little coverage of GArray behaviour with either of the zero_terminated or clear_ arguments to g_array_new() set to TRUE. Parameterise the tests and exhaustively expand the coverage to cover all possible GArray configurations. This was made possible by the online code coverage report for GLib which we now have: https://gnome.pages.gitlab.gnome.org/glib/coverage/glib/garray.c.gcov.html. Signed-off-by: Philip Withnall https://bugzilla.gnome.org/show_bug.cgi?id=795975 --- glib/tests/array-test.c | 309 +++++++++++++++++++++++++++++++++++----- 1 file changed, 276 insertions(+), 33 deletions(-) diff --git a/glib/tests/array-test.c b/glib/tests/array-test.c index 8c0872aef..e0a6109eb 100644 --- a/glib/tests/array-test.c +++ b/glib/tests/array-test.c @@ -30,6 +30,42 @@ #include #include "glib.h" +/* Test data to be passed to any function which calls g_array_new(), providing + * the parameters for that call. Most #GArray tests should be repeated for all + * possible values of #ArrayTestData. */ +typedef struct +{ + gboolean zero_terminated; + gboolean clear_; +} ArrayTestData; + +/* Assert that @garray contains @n_expected_elements as given in @expected_data. + * @garray must contain #gint elements. */ +static void +assert_int_array_equal (GArray *garray, + const gint *expected_data, + gsize n_expected_elements) +{ + gsize i; + + g_assert_cmpuint (garray->len, ==, n_expected_elements); + for (i = 0; i < garray->len; i++) + g_assert_cmpint (g_array_index (garray, gint, i), ==, expected_data[i]); +} + +/* Iff config->zero_terminated is %TRUE, assert that the final element of + * @garray is zero. @garray must contain #gint elements. */ +static void +assert_int_array_zero_terminated (const ArrayTestData *config, + GArray *garray) +{ + if (config->zero_terminated) + { + gint *data = (gint *) garray->data; + g_assert_cmpint (data[garray->len], ==, 0); + } +} + static void sum_up (gpointer data, gpointer user_data) @@ -42,38 +78,46 @@ sum_up (gpointer data, /* Check that expanding an array with g_array_set_size() clears the new elements * if @clear_ was specified during construction. */ static void -array_new_cleared (void) +array_set_size (gconstpointer test_data) { + const ArrayTestData *config = test_data; GArray *garray; gsize i; - garray = g_array_new (FALSE, TRUE, sizeof (gint)); + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); g_assert_cmpuint (garray->len, ==, 0); + assert_int_array_zero_terminated (config, garray); g_array_set_size (garray, 5); g_assert_cmpuint (garray->len, ==, 5); + assert_int_array_zero_terminated (config, garray); - for (i = 0; i < 5; i++) - g_assert_cmpint (g_array_index (garray, gint, i), ==, 0); + if (config->clear_) + for (i = 0; i < 5; i++) + g_assert_cmpint (g_array_index (garray, gint, i), ==, 0); g_array_unref (garray); } -/* As with array_new_cleared(), but with a sized array. */ +/* As with array_set_size(), but with a sized array. */ static void -array_new_sized_cleared (void) +array_set_size_sized (gconstpointer test_data) { + const ArrayTestData *config = test_data; GArray *garray; gsize i; - garray = g_array_sized_new (FALSE, TRUE, sizeof (gint), 10); + garray = g_array_sized_new (config->zero_terminated, config->clear_, sizeof (gint), 10); g_assert_cmpuint (garray->len, ==, 0); + assert_int_array_zero_terminated (config, garray); g_array_set_size (garray, 5); g_assert_cmpuint (garray->len, ==, 5); + assert_int_array_zero_terminated (config, garray); - for (i = 0; i < 5; i++) - g_assert_cmpint (g_array_index (garray, gint, i), ==, 0); + if (config->clear_) + for (i = 0; i < 5; i++) + g_assert_cmpint (g_array_index (garray, gint, i), ==, 0); g_array_unref (garray); } @@ -97,16 +141,20 @@ array_new_zero_terminated (void) g_free (out_str); } +/* Check that g_array_append_val() works correctly for various #GArray + * configurations. */ static void -array_append (void) +array_append_val (gconstpointer test_data) { + const ArrayTestData *config = test_data; GArray *garray; gint i; gint *segment; - garray = g_array_new (FALSE, FALSE, sizeof (gint)); + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); for (i = 0; i < 10000; i++) g_array_append_val (garray, i); + assert_int_array_zero_terminated (config, garray); for (i = 0; i < 10000; i++) g_assert_cmpint (g_array_index (garray, gint, i), ==, i); @@ -114,18 +162,25 @@ array_append (void) segment = (gint*)g_array_free (garray, FALSE); for (i = 0; i < 10000; i++) g_assert_cmpint (segment[i], ==, i); + if (config->zero_terminated) + g_assert_cmpint (segment[10000], ==, 0); + g_free (segment); } +/* Check that g_array_prepend_val() works correctly for various #GArray + * configurations. */ static void -array_prepend (void) +array_prepend_val (gconstpointer test_data) { + const ArrayTestData *config = test_data; GArray *garray; gint i; - garray = g_array_new (FALSE, FALSE, sizeof (gint)); + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); for (i = 0; i < 100; i++) g_array_prepend_val (garray, i); + assert_int_array_zero_terminated (config, garray); for (i = 0; i < 100; i++) g_assert_cmpint (g_array_index (garray, gint, i), ==, (100 - i - 1)); @@ -133,16 +188,141 @@ array_prepend (void) g_array_free (garray, TRUE); } +/* Test that g_array_prepend_vals() works correctly with various array + * configurations. */ static void -array_remove (void) +array_prepend_vals (gconstpointer test_data) { + const ArrayTestData *config = test_data; + GArray *garray, *garray_out; + const gint vals[] = { 0, 1, 2, 3, 4 }; + const gint expected_vals1[] = { 0, 1 }; + const gint expected_vals2[] = { 2, 0, 1 }; + const gint expected_vals3[] = { 3, 4, 2, 0, 1 }; + + /* Set up an array. */ + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); + assert_int_array_zero_terminated (config, garray); + + /* Prepend several values to an empty array. */ + garray_out = g_array_prepend_vals (garray, vals, 2); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals1, G_N_ELEMENTS (expected_vals1)); + assert_int_array_zero_terminated (config, garray); + + /* Prepend a single value. */ + garray_out = g_array_prepend_vals (garray, vals + 2, 1); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals2, G_N_ELEMENTS (expected_vals2)); + assert_int_array_zero_terminated (config, garray); + + /* Prepend several values to a non-empty array. */ + garray_out = g_array_prepend_vals (garray, vals + 3, 2); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals3, G_N_ELEMENTS (expected_vals3)); + assert_int_array_zero_terminated (config, garray); + + /* Prepend no values. */ + garray_out = g_array_prepend_vals (garray, vals, 0); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals3, G_N_ELEMENTS (expected_vals3)); + assert_int_array_zero_terminated (config, garray); + + /* Prepend no values with %NULL data. */ + garray_out = g_array_prepend_vals (garray, NULL, 0); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals3, G_N_ELEMENTS (expected_vals3)); + assert_int_array_zero_terminated (config, garray); + + g_array_free (garray, TRUE); +} + +/* Test that g_array_insert_vals() works correctly with various array + * configurations. */ +static void +array_insert_vals (gconstpointer test_data) +{ + const ArrayTestData *config = test_data; + GArray *garray, *garray_out; + gsize i; + const gint vals[] = { 0, 1, 2, 3, 4, 5, 6, 7 }; + const gint expected_vals1[] = { 0, 1 }; + const gint expected_vals2[] = { 0, 2, 3, 1 }; + const gint expected_vals3[] = { 0, 2, 3, 1, 4 }; + const gint expected_vals4[] = { 5, 0, 2, 3, 1, 4 }; + const gint expected_vals5[] = { 5, 0, 2, 3, 1, 4, 0, 0, 0, 0, 6, 7 }; + + /* Set up an array. */ + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); + assert_int_array_zero_terminated (config, garray); + + /* Insert several values at the beginning. */ + garray_out = g_array_insert_vals (garray, 0, vals, 2); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals1, G_N_ELEMENTS (expected_vals1)); + assert_int_array_zero_terminated (config, garray); + + /* Insert some more part-way through. */ + garray_out = g_array_insert_vals (garray, 1, vals + 2, 2); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals2, G_N_ELEMENTS (expected_vals2)); + assert_int_array_zero_terminated (config, garray); + + /* And at the end. */ + garray_out = g_array_insert_vals (garray, garray->len, vals + 4, 1); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals3, G_N_ELEMENTS (expected_vals3)); + assert_int_array_zero_terminated (config, garray); + + /* Then back at the beginning again. */ + garray_out = g_array_insert_vals (garray, 0, vals + 5, 1); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals4, G_N_ELEMENTS (expected_vals4)); + assert_int_array_zero_terminated (config, garray); + + /* Insert zero elements. */ + garray_out = g_array_insert_vals (garray, 0, vals, 0); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals4, G_N_ELEMENTS (expected_vals4)); + assert_int_array_zero_terminated (config, garray); + + /* Insert zero elements with a %NULL pointer. */ + garray_out = g_array_insert_vals (garray, 0, NULL, 0); + g_assert_true (garray == garray_out); + assert_int_array_equal (garray, expected_vals4, G_N_ELEMENTS (expected_vals4)); + assert_int_array_zero_terminated (config, garray); + + /* Insert some elements off the end of the array. The behaviour here depends + * on whether the array clears entries. */ + garray_out = g_array_insert_vals (garray, garray->len + 4, vals + 6, 2); + g_assert_true (garray == garray_out); + + g_assert_cmpuint (garray->len, ==, G_N_ELEMENTS (expected_vals5)); + for (i = 0; i < G_N_ELEMENTS (expected_vals5); i++) + { + if (config->clear_ || i < 6 || i > 9) + g_assert_cmpint (g_array_index (garray, gint, i), ==, expected_vals5[i]); + } + + assert_int_array_zero_terminated (config, garray); + + g_array_free (garray, TRUE); +} + +/* Check that g_array_remove_index() works correctly for various #GArray + * configurations. */ +static void +array_remove_index (gconstpointer test_data) +{ + const ArrayTestData *config = test_data; GArray *garray; gint i; gint prev, cur; - garray = g_array_new (FALSE, FALSE, sizeof (gint)); + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); for (i = 0; i < 100; i++) g_array_append_val (garray, i); + assert_int_array_zero_terminated (config, garray); g_assert_cmpint (garray->len, ==, 100); @@ -152,6 +332,7 @@ array_remove (void) g_array_remove_index (garray, 57); g_assert_cmpint (garray->len, ==, 96); + assert_int_array_zero_terminated (config, garray); prev = -1; for (i = 0; i < garray->len; i++) @@ -165,18 +346,22 @@ array_remove (void) g_array_free (garray, TRUE); } +/* Check that g_array_remove_index_fast() works correctly for various #GArray + * configurations. */ static void -array_remove_fast (void) +array_remove_index_fast (gconstpointer test_data) { + const ArrayTestData *config = test_data; GArray *garray; gint i; gint prev, cur; - garray = g_array_new (FALSE, FALSE, sizeof (gint)); + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); for (i = 0; i < 100; i++) g_array_append_val (garray, i); g_assert_cmpint (garray->len, ==, 100); + assert_int_array_zero_terminated (config, garray); g_array_remove_index_fast (garray, 1); g_array_remove_index_fast (garray, 3); @@ -184,6 +369,7 @@ array_remove_fast (void) g_array_remove_index_fast (garray, 57); g_assert_cmpint (garray->len, ==, 96); + assert_int_array_zero_terminated (config, garray); prev = -1; for (i = 0; i < garray->len; i++) @@ -200,22 +386,27 @@ array_remove_fast (void) g_array_free (garray, TRUE); } +/* Check that g_array_remove_range() works correctly for various #GArray + * configurations. */ static void -array_remove_range (void) +array_remove_range (gconstpointer test_data) { + const ArrayTestData *config = test_data; GArray *garray; gint i; gint prev, cur; - garray = g_array_new (FALSE, FALSE, sizeof (gint)); + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); for (i = 0; i < 100; i++) g_array_append_val (garray, i); g_assert_cmpint (garray->len, ==, 100); + assert_int_array_zero_terminated (config, garray); g_array_remove_range (garray, 31, 4); g_assert_cmpint (garray->len, ==, 96); + assert_int_array_zero_terminated (config, garray); prev = -1; for (i = 0; i < garray->len; i++) @@ -228,8 +419,15 @@ array_remove_range (void) /* Ensure the entire array can be cleared, even when empty. */ g_array_remove_range (garray, 0, garray->len); + + g_assert_cmpint (garray->len, ==, 0); + assert_int_array_zero_terminated (config, garray); + g_array_remove_range (garray, 0, garray->len); + g_assert_cmpint (garray->len, ==, 0); + assert_int_array_zero_terminated (config, garray); + g_array_free (garray, TRUE); } @@ -277,20 +475,27 @@ int_compare_data (gconstpointer p1, gconstpointer p2, gpointer data) return *i1 - *i2; } + +/* Check that g_array_sort() works correctly for various #GArray + * configurations. */ static void -array_sort (void) +array_sort (gconstpointer test_data) { + const ArrayTestData *config = test_data; GArray *garray; gint i; gint prev, cur; - garray = g_array_new (FALSE, FALSE, sizeof (gint)); + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); for (i = 0; i < 10000; i++) { cur = g_random_int_range (0, 10000); g_array_append_val (garray, cur); } - g_array_sort (garray, int_compare); + assert_int_array_zero_terminated (config, garray); + + g_array_sort (garray, int_compare); + assert_int_array_zero_terminated (config, garray); prev = -1; for (i = 0; i < garray->len; i++) @@ -303,20 +508,26 @@ array_sort (void) g_array_free (garray, TRUE); } +/* Check that g_array_sort_with_data() works correctly for various #GArray + * configurations. */ static void -array_sort_with_data (void) +array_sort_with_data (gconstpointer test_data) { + const ArrayTestData *config = test_data; GArray *garray; gint i; gint prev, cur; - garray = g_array_new (FALSE, FALSE, sizeof (gint)); + garray = g_array_new (config->zero_terminated, config->clear_, sizeof (gint)); for (i = 0; i < 10000; i++) { cur = g_random_int_range (0, 10000); g_array_append_val (garray, cur); } + assert_int_array_zero_terminated (config, garray); + g_array_sort_with_data (garray, int_compare_data, NULL); + assert_int_array_zero_terminated (config, garray); prev = -1; for (i = 0; i < garray->len; i++) @@ -993,27 +1204,59 @@ byte_array_free_to_bytes (void) g_bytes_unref (bytes); } + +static void +add_array_test (const gchar *test_path, + const ArrayTestData *config, + GTestDataFunc test_func) +{ + gchar *test_name = NULL; + + test_name = g_strdup_printf ("%s/%s-%s", + test_path, + config->zero_terminated ? "zero-terminated" : "non-zero-terminated", + config->clear_ ? "clear" : "no-clear"); + g_test_add_data_func (test_name, config, test_func); + g_free (test_name); +} + int main (int argc, char *argv[]) { + /* Test all possible combinations of g_array_new() parameters. */ + const ArrayTestData array_configurations[] = + { + { FALSE, FALSE }, + { FALSE, TRUE }, + { TRUE, FALSE }, + { TRUE, TRUE }, + }; + gsize i; + g_test_init (&argc, &argv, NULL); g_test_bug_base ("https://bugzilla.gnome.org/"); /* array tests */ - g_test_add_func ("/array/new/cleared", array_new_cleared); - g_test_add_func ("/array/new/sized-cleared", array_new_sized_cleared); g_test_add_func ("/array/new/zero-terminated", array_new_zero_terminated); - g_test_add_func ("/array/append", array_append); - g_test_add_func ("/array/prepend", array_prepend); - g_test_add_func ("/array/remove", array_remove); - g_test_add_func ("/array/remove-fast", array_remove_fast); - g_test_add_func ("/array/remove-range", array_remove_range); g_test_add_func ("/array/ref-count", array_ref_count); - g_test_add_func ("/array/sort", array_sort); - g_test_add_func ("/array/sort-with-data", array_sort_with_data); g_test_add_func ("/array/clear-func", array_clear_func); + for (i = 0; i < G_N_ELEMENTS (array_configurations); i++) + { + add_array_test ("/array/set-size", &array_configurations[i], array_set_size); + add_array_test ("/array/set-size/sized", &array_configurations[i], array_set_size_sized); + add_array_test ("/array/append-val", &array_configurations[i], array_append_val); + add_array_test ("/array/prepend-val", &array_configurations[i], array_prepend_val); + add_array_test ("/array/prepend-vals", &array_configurations[i], array_prepend_vals); + add_array_test ("/array/insert-vals", &array_configurations[i], array_insert_vals); + add_array_test ("/array/remove-index", &array_configurations[i], array_remove_index); + add_array_test ("/array/remove-index-fast", &array_configurations[i], array_remove_index_fast); + add_array_test ("/array/remove-range", &array_configurations[i], array_remove_range); + add_array_test ("/array/sort", &array_configurations[i], array_sort); + add_array_test ("/array/sort-with-data", &array_configurations[i], array_sort_with_data); + } + /* pointer arrays */ g_test_add_func ("/pointerarray/add", pointer_array_add); g_test_add_func ("/pointerarray/insert", pointer_array_insert);