From 55d01904f1010a6257c5e30c3095b2290d5f48f8 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 15 Aug 2024 10:56:03 +1000 Subject: [PATCH 1/8] run-full-test: use meson setup builddir Instead of the deprecated `meson builddir` --- run-full-test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/run-full-test.sh b/run-full-test.sh index 28c97183c..07b3afb1e 100755 --- a/run-full-test.sh +++ b/run-full-test.sh @@ -6,7 +6,7 @@ date=`date +"%Y-%m-%d-%H.%M.%S"` builddir="build.$date" echo "####################################### running test suite" -meson $builddir +meson setup $builddir ninja -C $builddir test echo "####################################### running valgrind" From ed9e492383693878b580ff70e3915e2e6c241cfb Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 15 Aug 2024 10:08:38 +1000 Subject: [PATCH 2/8] tools: silence a deadstore compiler warning We could move this into the respective default: case of the switch statement below but then we won't get compiler warnings about missing enum values. So let's add this to have a clean scan-build output. --- tools/list-local-devices.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/list-local-devices.c b/tools/list-local-devices.c index f62251883..ea39b2579 100644 --- a/tools/list-local-devices.c +++ b/tools/list-local-devices.c @@ -151,6 +151,10 @@ tablet_print_yaml(gpointer data, gpointer user_data) WacomEraserType eraser_type = libwacom_stylus_get_eraser_type(stylus); const char *etype= "unknown"; + /* warning: Value stored to 'type' during its initialization is never read [deadcode.DeadStores] */ + (void)type; + (void)etype; + switch (libwacom_stylus_get_type(stylus)) { case WSTYLUS_UNKNOWN: type = "unknown"; break; case WSTYLUS_GENERAL: type = "general"; break; From 48d0e27230991c600cc6c7a374b6fb3f7014e46d Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Tue, 13 Aug 2024 12:03:18 +1000 Subject: [PATCH 3/8] database: build a default Styli tablet file entry Instead of duplicating the code here build the default Styli= entry and parse it as if it were set by the tablet file. --- libwacom/libwacom-database.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/libwacom/libwacom-database.c b/libwacom/libwacom-database.c index eb4751096..6f98bae36 100644 --- a/libwacom/libwacom-database.c +++ b/libwacom/libwacom-database.c @@ -900,16 +900,13 @@ libwacom_parse_tablet_keyfile(WacomDeviceDatabase *db, g_free(class); string_list = g_key_file_get_string_list(keyfile, DEVICE_GROUP, "Styli", NULL, NULL); - if (string_list) { - libwacom_parse_styli_list(db, device, string_list); - g_strfreev (string_list); - } else { - int fallback_eraser = WACOM_ERASER_FALLBACK_ID; - int fallback_stylus = WACOM_STYLUS_FALLBACK_ID; - device->styli = g_array_new(FALSE, FALSE, sizeof(int)); - g_array_append_val(device->styli, fallback_eraser); - g_array_append_val(device->styli, fallback_stylus); + if (!string_list) { + string_list = g_new0(char*, 3); + string_list[0] = g_strdup_printf("0x%x", WACOM_ERASER_FALLBACK_ID); + string_list[1] = g_strdup_printf("0x%x", WACOM_STYLUS_FALLBACK_ID); } + libwacom_parse_styli_list(db, device, string_list); + g_strfreev (string_list); device->num_strips = g_key_file_get_integer(keyfile, FEATURES_GROUP, "NumStrips", NULL); device->num_dials = g_key_file_get_integer(keyfile, FEATURES_GROUP, "NumDials", NULL); From 495a4416feadface61229d474f4d8b7590984af1 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 15 Aug 2024 10:53:49 +1000 Subject: [PATCH 4/8] Use g_array_copy instead of an open-coded copy --- libwacom/libwacom.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/libwacom/libwacom.c b/libwacom/libwacom.c index 1ffb910a2..afe9c0303 100644 --- a/libwacom/libwacom.c +++ b/libwacom/libwacom.c @@ -400,12 +400,10 @@ libwacom_copy(const WacomDevice *device) d->height = device->height; d->integration_flags = device->integration_flags; d->layout = g_strdup(device->layout); - d->matches = g_array_sized_new(TRUE, TRUE, sizeof(WacomDevice*), - device->matches->len); + d->matches = g_array_copy(device->matches); for (guint i = 0; i < device->matches->len; i++) { - WacomMatch *m = g_array_index(device->matches, WacomMatch*, i); + WacomMatch *m = g_array_index(d->matches, WacomMatch*, i); libwacom_match_ref(m); - g_array_append_val(d->matches, m); } d->match = libwacom_match_ref(device->match); if (device->paired) @@ -420,20 +418,8 @@ libwacom_copy(const WacomDevice *device) d->dial2_num_modes = device->dial2_num_modes; d->ring_num_modes = device->ring_num_modes; d->ring2_num_modes = device->ring2_num_modes; - d->styli = g_array_sized_new(FALSE, FALSE, sizeof(int), - device->styli->len); - for (guint i = 0; i < device->styli->len; i++) { - int id = g_array_index(device->styli, int, i); - g_array_append_val(d->styli, id); - } - d->status_leds = g_array_sized_new(FALSE, FALSE, - sizeof(WacomStatusLEDs), - device->status_leds->len); - for (guint i = 0; i < device->status_leds->len; i++) { - g_array_append_val(d->status_leds, - g_array_index(device->status_leds, WacomStatusLEDs, i)); - } - + d->styli = g_array_copy(device->styli); + d->status_leds = g_array_copy(device->status_leds); d->buttons = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_free); g_hash_table_iter_init(&iter, device->buttons); From ed48e6ff6f4e230b170770bbe5c538c36e81aab2 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 15 Aug 2024 11:53:12 +1000 Subject: [PATCH 5/8] Use g_array_element_size instead of a sizeof for array comparison Ensure the memcmp works on the array element size without having to remember what exactly that was. --- libwacom/libwacom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libwacom/libwacom.c b/libwacom/libwacom.c index afe9c0303..40b54b705 100644 --- a/libwacom/libwacom.c +++ b/libwacom/libwacom.c @@ -555,7 +555,7 @@ libwacom_compare(const WacomDevice *a, const WacomDevice *b, WacomCompareFlags f if (a->status_leds->len > 0 && memcmp(a->status_leds->data, b->status_leds->data, - sizeof(WacomStatusLEDs) * a->status_leds->len) != 0) + g_array_get_element_size(a->status_leds) * a->status_leds->len) != 0) return 1; g_hash_table_iter_init(&iter, a->buttons); From b304c99deb353224e3c0bd3211009ba70a3423f7 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Tue, 13 Aug 2024 15:59:49 +1000 Subject: [PATCH 6/8] test: reword a test using glibs' helpers instead of manual resizing Use g_hash_table_add() to treat it like a set and then copy that over into a GArray. --- test/test-stylus-validity.c | 41 ++++++++----------------------------- 1 file changed, 9 insertions(+), 32 deletions(-) diff --git a/test/test-stylus-validity.c b/test/test-stylus-validity.c index 850a40b66..e17d91120 100644 --- a/test/test-stylus-validity.c +++ b/test/test-stylus-validity.c @@ -445,10 +445,8 @@ static const WacomStylus ** assemble_styli(WacomDeviceDatabase *db) { WacomDevice **devices = libwacom_list_devices_from_database(db, NULL); - const WacomStylus **styli; - int *ids = NULL; - int nids = 0; - int sz = 0; + GHashTable *all = g_hash_table_new(g_direct_hash, g_direct_equal); + const WacomStylus **all_styli = NULL; g_assert(devices); @@ -457,39 +455,18 @@ assemble_styli(WacomDeviceDatabase *db) int nstyli; styli = libwacom_get_supported_styli(*d, &nstyli); - - /* Make sure our array is large enough to accommodate for - all new styli. Simpler than reallocing after every entry */ - if (nstyli > sz - nids) { - sz = nids + nstyli; - ids = realloc(ids, sz * sizeof(*ids)); - g_assert(ids); - } - - /* For each stylus in the current device, add it to ids[] if - it's not already in there */ for (int i = 0; i < nstyli; i++) { - gboolean found = FALSE; - - for (int j = 0; j < nids && !found; j++) { - if (ids[j] == styli[i]) - found = TRUE; - } - - if (!found) - ids[nids++] = styli[i]; + const WacomStylus *stylus = libwacom_stylus_get_for_id (db, styli[i]); + g_hash_table_add(all, (gpointer)stylus); } } - styli = calloc(nids + 1, sizeof(*styli)); - for (int i = 0; i < nids; i++) { - styli[i] = libwacom_stylus_get_for_id (db, ids[i]); - g_assert(styli[i]); - } + all_styli = (const WacomStylus**)g_hash_table_get_keys_as_array(all, NULL); + g_hash_table_steal_all(all); + g_clear_pointer(&all, g_hash_table_unref); + g_clear_pointer(&devices, g_free); - free(devices); - free(ids); - return styli; + return all_styli; } static WacomDeviceDatabase * From 1a7c4d59b45c79af2401310d8b65f649d2c71963 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 15 Aug 2024 10:07:46 +1000 Subject: [PATCH 7/8] database: split a re-used variable and use g_clear_pointer Slightly improved code-hygiene, we can afford an extra pointer so we don't accidentally re-use a variable. And g_clear_pointer() resets everything to NULL so re-using it will blow up instead of giving us invalid values. --- libwacom/libwacom-database.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/libwacom/libwacom-database.c b/libwacom/libwacom-database.c index 6f98bae36..ac7883335 100644 --- a/libwacom/libwacom-database.c +++ b/libwacom/libwacom-database.c @@ -270,7 +270,7 @@ libwacom_parse_stylus_keyfile(WacomDeviceDatabase *db, const char *path) for (i = 0; groups[i]; i++) { WacomStylus *stylus; GError *error = NULL; - char *type; + char *eraser_type, *type; int id; char **string_list; @@ -285,9 +285,9 @@ libwacom_parse_stylus_keyfile(WacomDeviceDatabase *db, const char *path) stylus->name = g_key_file_get_string(keyfile, groups[i], "Name", NULL); stylus->group = g_key_file_get_string(keyfile, groups[i], "Group", NULL); - type = g_key_file_get_string(keyfile, groups[i], "EraserType", NULL); - stylus->eraser_type = eraser_type_from_str (type); - g_free (type); + eraser_type = g_key_file_get_string(keyfile, groups[i], "EraserType", NULL); + stylus->eraser_type = eraser_type_from_str (eraser_type); + g_clear_pointer(&eraser_type, g_free); string_list = g_key_file_get_string_list (keyfile, groups[i], "PairedStylusIds", NULL, NULL); stylus->paired_ids = g_array_new (FALSE, FALSE, sizeof(int)); @@ -355,16 +355,15 @@ libwacom_parse_stylus_keyfile(WacomDeviceDatabase *db, const char *path) type = g_key_file_get_string(keyfile, groups[i], "Type", NULL); stylus->type = type_from_str (type); - g_free (type); + g_clear_pointer(&type, g_free); if (g_hash_table_lookup (db->stylus_ht, GINT_TO_POINTER (id)) != NULL) g_warning ("Duplicate definition for stylus ID '%#x'", id); g_hash_table_insert (db->stylus_ht, GINT_TO_POINTER (id), stylus); } - g_strfreev (groups); - if (keyfile) - g_key_file_free (keyfile); + g_clear_pointer(&groups, g_strfreev); + g_clear_pointer(&keyfile, g_key_file_free); } static void From fd67342ed344098116a9b331f54cff31bee4629f Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 15 Aug 2024 09:55:05 +1000 Subject: [PATCH 8/8] Reduce nesting by one level for the string list Instead of if (string_list) { for (....) { Do this for (...; string_list && ...) { We can move the string_list != NULL check into the for loop. And use g_clear_pointer() to ensure our pointer is NULL in case we accidentally re-use it. No functional changes. --- libwacom/libwacom-database.c | 69 +++++++++++++++--------------------- 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/libwacom/libwacom-database.c b/libwacom/libwacom-database.c index ac7883335..050ee2551 100644 --- a/libwacom/libwacom-database.c +++ b/libwacom/libwacom-database.c @@ -289,23 +289,18 @@ libwacom_parse_stylus_keyfile(WacomDeviceDatabase *db, const char *path) stylus->eraser_type = eraser_type_from_str (eraser_type); g_clear_pointer(&eraser_type, g_free); - string_list = g_key_file_get_string_list (keyfile, groups[i], "PairedStylusIds", NULL, NULL); stylus->paired_ids = g_array_new (FALSE, FALSE, sizeof(int)); - if (string_list) { - guint j; - - for (j = 0; string_list[j]; j++) { - int val; + string_list = g_key_file_get_string_list (keyfile, groups[i], "PairedStylusIds", NULL, NULL); + for (guint j = 0; string_list && string_list[j]; j++) { + int val; - if (safe_atoi_base (string_list[j], &val, 16)) { - g_array_append_val (stylus->paired_ids, val); - } else { - g_warning ("Stylus %s (%s) Ignoring invalid PairedStylusIds value\n", stylus->name, groups[i]); - } + if (safe_atoi_base (string_list[j], &val, 16)) { + g_array_append_val (stylus->paired_ids, val); + } else { + g_warning ("Stylus %s (%s) Ignoring invalid PairedStylusIds value\n", stylus->name, groups[i]); } - - g_strfreev (string_list); } + g_clear_pointer(&string_list, g_strfreev); stylus->has_lens = g_key_file_get_boolean(keyfile, groups[i], "HasLens", &error); if (error && error->code == G_KEY_FILE_ERROR_INVALID_VALUE) @@ -322,36 +317,30 @@ libwacom_parse_stylus_keyfile(WacomDeviceDatabase *db, const char *path) g_clear_error (&error); } + stylus->axes = WACOM_AXIS_TYPE_NONE; string_list = g_key_file_get_string_list (keyfile, groups[i], "Axes", NULL, NULL); - if (string_list) { - WacomAxisTypeFlags axes = WACOM_AXIS_TYPE_NONE; - guint j; - - for (j = 0; string_list[j]; j++) { - WacomAxisTypeFlags flag = WACOM_AXIS_TYPE_NONE; - if (g_str_equal(string_list[j], "Tilt")) { - flag = WACOM_AXIS_TYPE_TILT; - } else if (g_str_equal(string_list[j], "RotationZ")) { - flag = WACOM_AXIS_TYPE_ROTATION_Z; - } else if (g_str_equal(string_list[j], "Distance")) { - flag = WACOM_AXIS_TYPE_DISTANCE; - } else if (g_str_equal(string_list[j], "Pressure")) { - flag = WACOM_AXIS_TYPE_PRESSURE; - } else if (g_str_equal(string_list[j], "Slider")) { - flag = WACOM_AXIS_TYPE_SLIDER; - } else { - g_warning ("Invalid axis %s for stylus ID %s\n", - string_list[j], groups[i]); - } - if (axes & flag) - g_warning ("Duplicate axis %s for stylus ID %s\n", - string_list[j], groups[i]); - axes |= flag; + for (guint j = 0; string_list && string_list[j]; j++) { + WacomAxisTypeFlags flag = WACOM_AXIS_TYPE_NONE; + if (g_str_equal(string_list[j], "Tilt")) { + flag = WACOM_AXIS_TYPE_TILT; + } else if (g_str_equal(string_list[j], "RotationZ")) { + flag = WACOM_AXIS_TYPE_ROTATION_Z; + } else if (g_str_equal(string_list[j], "Distance")) { + flag = WACOM_AXIS_TYPE_DISTANCE; + } else if (g_str_equal(string_list[j], "Pressure")) { + flag = WACOM_AXIS_TYPE_PRESSURE; + } else if (g_str_equal(string_list[j], "Slider")) { + flag = WACOM_AXIS_TYPE_SLIDER; + } else { + g_warning ("Invalid axis %s for stylus ID %s\n", + string_list[j], groups[i]); } - - stylus->axes = axes; - g_strfreev (string_list); + if (stylus->axes & flag) + g_warning ("Duplicate axis %s for stylus ID %s\n", + string_list[j], groups[i]); + stylus->axes |= flag; } + g_clear_pointer(&string_list, g_strfreev); type = g_key_file_get_string(keyfile, groups[i], "Type", NULL); stylus->type = type_from_str (type);