From e84549926bbeaf6f19428f1aeac1b96eeb860b6e Mon Sep 17 00:00:00 2001 From: Christopher Lam Date: Tue, 10 Aug 2021 12:55:47 +0800 Subject: [PATCH 1/3] [gnc-glib-utils] gnc_g_list_stringjoin to join a GList of strings It traverses the GList twice (once to calculate the length) but allocates only once. Includes snippet from https://www.joelonsoftware.com/2001/12/11/back-to-basics/ --- libgnucash/core-utils/gnc-glib-utils.c | 32 +++++++++++ libgnucash/core-utils/gnc-glib-utils.h | 16 ++++++ .../core-utils/test/test-gnc-glib-utils.c | 57 +++++++++++++++++++ 3 files changed, 105 insertions(+) diff --git a/libgnucash/core-utils/gnc-glib-utils.c b/libgnucash/core-utils/gnc-glib-utils.c index b452921511..c15e1dd8d8 100644 --- a/libgnucash/core-utils/gnc-glib-utils.c +++ b/libgnucash/core-utils/gnc-glib-utils.c @@ -327,3 +327,35 @@ void gnc_gpid_kill(GPid pid) } #endif /* G_OS_WIN32 */ } + +static inline char* +gnc_strcat (char* dest, const char* src) +{ + while (*dest) dest++; + while ((*dest++ = *src++)); + return --dest; +} + +gchar * +gnc_g_list_stringjoin (GList *list_of_strings, const gchar *sep) +{ + gint seplen = sep ? strlen(sep) : 0; + gint length = -seplen; + gchar *retval, *p; + + if (!list_of_strings) + return NULL; + + for (GList *n = list_of_strings; n; n = n->next) + length += strlen ((gchar*)n->data) + seplen; + + p = retval = (gchar*) g_malloc0 (length * sizeof (gchar) + 1); + for (GList *n = list_of_strings; n; n = n->next) + { + p = gnc_strcat (p, (gchar*)n->data); + if (n->next && sep) + p = gnc_strcat (p, sep); + } + + return retval; +} diff --git a/libgnucash/core-utils/gnc-glib-utils.h b/libgnucash/core-utils/gnc-glib-utils.h index 7f1d1dd2ef..97e36c53d4 100644 --- a/libgnucash/core-utils/gnc-glib-utils.h +++ b/libgnucash/core-utils/gnc-glib-utils.h @@ -183,6 +183,22 @@ void gnc_scm_log_debug(const gchar *msg); @{ */ + +/** + * @brief Return a string joining a GList whose elements are gchar* + * strings. Returns NULL if an empty GList* is passed through. The + * optional sep string will be used as separator. Must be g_freed when + * not needed anymore. + * + * @param list_of_strings A GList of chars* + * + * @param sep a separator or NULL + * + * @return A newly allocated string that has to be g_free'd by the + * caller. + **/ +gchar * gnc_g_list_stringjoin (GList *list_of_strings, const gchar *sep); + /** Kill a process. On UNIX send a SIGKILL, on Windows call TerminateProcess. * * @param pid The process ID. */ diff --git a/libgnucash/core-utils/test/test-gnc-glib-utils.c b/libgnucash/core-utils/test/test-gnc-glib-utils.c index 390d6b0fa1..e4423baaac 100644 --- a/libgnucash/core-utils/test/test-gnc-glib-utils.c +++ b/libgnucash/core-utils/test/test-gnc-glib-utils.c @@ -53,6 +53,62 @@ test_gnc_utf8_strip_invalid_and_controls (gconstpointer data) g_free (msg1); } +static void +test_g_list_stringjoin (gconstpointer data) +{ + GList *test = NULL; + gchar *ret; + + ret = gnc_g_list_stringjoin (NULL, NULL); + g_assert (ret == NULL); + + ret = gnc_g_list_stringjoin (NULL, ":"); + g_assert (ret == NULL); + + test = g_list_prepend (test, "one"); + + ret = gnc_g_list_stringjoin (test, NULL); + g_assert_cmpstr (ret, ==, "one"); + g_free (ret); + + ret = gnc_g_list_stringjoin (test, ""); + g_assert_cmpstr (ret, ==, "one"); + g_free (ret); + + ret = gnc_g_list_stringjoin (test, ":"); + g_assert_cmpstr (ret, ==, "one"); + g_free (ret); + + test = g_list_prepend (test, "two"); + + ret = gnc_g_list_stringjoin (test, NULL); + g_assert_cmpstr (ret, ==, "twoone"); + g_free (ret); + + ret = gnc_g_list_stringjoin (test, ""); + g_assert_cmpstr (ret, ==, "twoone"); + g_free (ret); + + ret = gnc_g_list_stringjoin (test, ":"); + g_assert_cmpstr (ret, ==, "two:one"); + g_free (ret); + + test = g_list_prepend (test, "three"); + + ret = gnc_g_list_stringjoin (test, NULL); + g_assert_cmpstr (ret, ==, "threetwoone"); + g_free (ret); + + ret = gnc_g_list_stringjoin (test, ""); + g_assert_cmpstr (ret, ==, "threetwoone"); + g_free (ret); + + ret = gnc_g_list_stringjoin (test, ":"); + g_assert_cmpstr (ret, ==, "three:two:one"); + g_free (ret); + + g_list_free (test); +} int main (int argc, char *argv[]) @@ -62,6 +118,7 @@ main (int argc, char *argv[]) g_test_init (&argc, &argv, NULL); // initialize test program g_test_add_data_func ("/core-utils/gnc_utf8_strip_invalid_and_controls invalid utf8", (gconstpointer)invalid_utf8, test_gnc_utf8_strip_invalid_and_controls); g_test_add_data_func ("/core-utils/gnc_utf8_strip_invalid_and_controls control chars", (gconstpointer)controls, test_gnc_utf8_strip_invalid_and_controls); + g_test_add_data_func ("/core-utils/gnc_g_list_stringjoin", NULL, test_g_list_stringjoin); return g_test_run(); } From 4c37f6d4ef1de455dd24839ab4a126c6f71c6c70 Mon Sep 17 00:00:00 2001 From: Christopher Lam Date: Wed, 11 Aug 2021 09:28:22 +0800 Subject: [PATCH 2/3] [account.cpp] gnc_g_list_stringjoin instead of repeated allocations --- libgnucash/engine/Account.cpp | 16 +--------------- libgnucash/engine/test/utest-Account.cpp | 16 +++------------- 2 files changed, 4 insertions(+), 28 deletions(-) diff --git a/libgnucash/engine/Account.cpp b/libgnucash/engine/Account.cpp index 78755e38d5..ba09427581 100644 --- a/libgnucash/engine/Account.cpp +++ b/libgnucash/engine/Account.cpp @@ -227,26 +227,12 @@ gnc_set_account_separator (const gchar *separator) gchar *gnc_account_name_violations_errmsg (const gchar *separator, GList* invalid_account_names) { - GList *node; gchar *message = NULL; - gchar *account_list = NULL; if ( !invalid_account_names ) return NULL; - for ( node = invalid_account_names; node; node = g_list_next(node)) - { - if ( !account_list ) - account_list = static_cast(node->data); - else - { - gchar *tmp_list = NULL; - - tmp_list = g_strconcat (account_list, "\n", node->data, nullptr); - g_free ( account_list ); - account_list = tmp_list; - } - } + auto account_list {gnc_g_list_stringjoin (invalid_account_names, "\n")}; /* Translators: The first %s will be the account separator character, the second %s is a list of account names. diff --git a/libgnucash/engine/test/utest-Account.cpp b/libgnucash/engine/test/utest-Account.cpp index b54163cd0f..8deea5fcfd 100644 --- a/libgnucash/engine/test/utest-Account.cpp +++ b/libgnucash/engine/test/utest-Account.cpp @@ -29,6 +29,7 @@ extern "C" #include #include /* Add specific headers for this class */ +#include "gnc-glib-utils.h" #include "../Account.h" #include "../AccountP.h" #include "../Split.h" @@ -429,24 +430,12 @@ test_gnc_account_name_violations_errmsg () { GList *badnames = NULL, *nonames = NULL, *node = NULL; auto separator = ":"; - char *account_list = NULL; /* FUT wants to free the strings, so we alloc them */ badnames = g_list_prepend (badnames, g_strdup ("Foo:bar")); badnames = g_list_prepend (badnames, g_strdup ("baz")); badnames = g_list_prepend (badnames, g_strdup ("waldo:pepper")); auto message = gnc_account_name_violations_errmsg (separator, nonames); - for (node = badnames; node; node = g_list_next (node)) - { - if (!account_list) - account_list = g_strdup (static_cast(node->data)); - else - { - auto tmp_list = g_strconcat ( account_list, "\n", - static_cast(node->data), NULL); - g_free (account_list); - account_list = tmp_list; - } - } + auto account_list = gnc_g_list_stringjoin (badnames, "\n"); message = gnc_account_name_violations_errmsg (separator, nonames); g_assert (message == NULL); auto validation_message = g_strdup_printf ( @@ -455,6 +444,7 @@ test_gnc_account_name_violations_errmsg () "Either change the account names or choose another separator " "character.\n\nBelow you will find the list of invalid account names:\n" "%s", separator, account_list); + g_free (account_list); message = gnc_account_name_violations_errmsg (separator, badnames); g_assert_cmpstr ( message, == , validation_message); g_free (validation_message); From 40d886fa9d54d556efbd6f150fd82a5a92d48fd1 Mon Sep 17 00:00:00 2001 From: Christopher Lam Date: Wed, 11 Aug 2021 09:28:26 +0800 Subject: [PATCH 3/3] gnc_account_list_name_violations elements must be freed --- gnucash/gnome-utils/dialog-preferences.c | 2 +- gnucash/gnome-utils/gnc-file.c | 1 + libgnucash/engine/Account.h | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/gnucash/gnome-utils/dialog-preferences.c b/gnucash/gnome-utils/dialog-preferences.c index 2e374bdeaa..62d40ddcd1 100644 --- a/gnucash/gnome-utils/dialog-preferences.c +++ b/gnucash/gnome-utils/dialog-preferences.c @@ -132,7 +132,7 @@ static gchar *gnc_account_separator_is_valid (const gchar *separator, message = gnc_account_name_violations_errmsg (*normalized_separator, conflict_accts); - g_list_free (conflict_accts); + g_list_free_full (conflict_accts, g_free); return message; } diff --git a/gnucash/gnome-utils/gnc-file.c b/gnucash/gnome-utils/gnc-file.c index d76698e595..677125c1e9 100644 --- a/gnucash/gnome-utils/gnc-file.c +++ b/gnucash/gnome-utils/gnc-file.c @@ -1101,6 +1101,7 @@ RESTART: invalid_account_names ); gnc_warning_dialog(parent, "%s", message); g_free ( message ); + g_list_free_full (invalid_account_names, g_free); } // Fix account color slots being set to 'Not Set', should run once on a book diff --git a/libgnucash/engine/Account.h b/libgnucash/engine/Account.h index 910d96fdf9..72f525a03b 100644 --- a/libgnucash/engine/Account.h +++ b/libgnucash/engine/Account.h @@ -283,8 +283,8 @@ gchar *gnc_account_name_violations_errmsg (const gchar *separator, GList* invali * @param book Pointer to the book with accounts to verify * @param separator The separator character to verify against * - * @return A GList of invalid account names. Should be freed with g_list_free - * if no longer needed. + * @return A GList of invalid account names. Should be freed with + * g_list_free_full (value, g_free) when no longer needed. */ GList *gnc_account_list_name_violations (QofBook *book, const gchar *separator);