From 9079d87307c3a1b7e7d33c64c8ed30ebe7c29d7c Mon Sep 17 00:00:00 2001 From: Stefan Koch Date: Thu, 15 Jan 2026 08:41:35 -0500 Subject: [PATCH 1/7] Remove unused qof_collection_from_glist function. --- libgnucash/engine/qofid.cpp | 20 -------------------- libgnucash/engine/qofid.h | 13 ------------- 2 files changed, 33 deletions(-) diff --git a/libgnucash/engine/qofid.cpp b/libgnucash/engine/qofid.cpp index 78fefff1df..2d3bf402e6 100644 --- a/libgnucash/engine/qofid.cpp +++ b/libgnucash/engine/qofid.cpp @@ -220,26 +220,6 @@ qof_collection_lookup_entity (const QofCollection *col, const GncGUID * guid) return ent; } -QofCollection * -qof_collection_from_glist (QofIdType type, const GList *glist) -{ - QofCollection *coll; - QofInstance *ent; - const GList *list; - - coll = qof_collection_new(type); - for (list = glist; list != NULL; list = list->next) - { - ent = QOF_INSTANCE(list->data); - if (FALSE == qof_collection_add_entity(coll, ent)) - { - qof_collection_destroy(coll); - return NULL; - } - } - return coll; -} - guint qof_collection_count (const QofCollection *col) { diff --git a/libgnucash/engine/qofid.h b/libgnucash/engine/qofid.h index 8a6f6157fa..b3eabbdf45 100644 --- a/libgnucash/engine/qofid.h +++ b/libgnucash/engine/qofid.h @@ -203,19 +203,6 @@ not in the other. gint qof_collection_compare (QofCollection *target, QofCollection *merge); -/** \brief Create a secondary collection from a GList - -@param type The QofIdType of the QofCollection \b and of - \b all entities in the GList. -@param glist GList of entities of the same QofIdType. - -@return NULL if any of the entities fail to match the - QofCollection type, else a pointer to the collection - on success. -*/ -QofCollection* -qof_collection_from_glist (QofIdType type, const GList *glist); - /** @} */ /** @} */ From a2cc9881dfc08ebde84ce0b9a421b9f2d7e5d708 Mon Sep 17 00:00:00 2001 From: Stefan Koch Date: Thu, 15 Jan 2026 08:42:20 -0500 Subject: [PATCH 2/7] Explicitly set the is_dirty attribute in qof_collection_new It is likely that the memory was zeroed anyway, but the explicitness makes it easier to read. --- libgnucash/engine/qofid.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libgnucash/engine/qofid.cpp b/libgnucash/engine/qofid.cpp index 2d3bf402e6..b1bf7e8e50 100644 --- a/libgnucash/engine/qofid.cpp +++ b/libgnucash/engine/qofid.cpp @@ -50,6 +50,7 @@ qof_collection_new (QofIdType type) QofCollection *col; col = g_new0(QofCollection, 1); col->e_type = static_cast(CACHE_INSERT (type)); + col->is_dirty = FALSE; col->hash_of_entities = guid_hash_table_new(); col->data = NULL; return col; From 6e9a20dfebca6bbba8d9cfdeb6b4183a3d0fe364 Mon Sep 17 00:00:00 2001 From: Stefan Koch Date: Tue, 13 Jan 2026 12:30:22 -0500 Subject: [PATCH 3/7] Implement full test coverage of libgnucash/engine/qofid NOTE: This does not have full coverage because the "if (!target || !ent)" body in the collection_compare_cp function cannot be reached. There is too much safety in the rest of the system to test this error. NOTE: I made this a separate test from the test-engine so that I was able to check that it by itself could test the full coverage of the qofid.cpp file. If it was part of the larger test, I could have missed some parts that were covered incidentally elsewhere. --- libgnucash/engine/test/CMakeLists.txt | 3 + libgnucash/engine/test/test-qofid.cpp | 205 ++++++++++++++++++++++++++ 2 files changed, 208 insertions(+) create mode 100644 libgnucash/engine/test/test-qofid.cpp diff --git a/libgnucash/engine/test/CMakeLists.txt b/libgnucash/engine/test/CMakeLists.txt index c920f0d38a..137ad90252 100644 --- a/libgnucash/engine/test/CMakeLists.txt +++ b/libgnucash/engine/test/CMakeLists.txt @@ -130,6 +130,9 @@ set(test_qofsession_SOURCES gnc_add_test(test-qofsession "${test_qofsession_SOURCES}" gtest_engine_INCLUDES gtest_old_engine_LIBS) +gnc_add_test(test-qofid test-qofid.cpp + gtest_engine_INCLUDES gtest_old_engine_LIBS) + gnc_add_test(test-gnc-euro gtest-gnc-euro.cpp gtest_engine_INCLUDES gtest_old_engine_LIBS) diff --git a/libgnucash/engine/test/test-qofid.cpp b/libgnucash/engine/test/test-qofid.cpp new file mode 100644 index 0000000000..9897a87eee --- /dev/null +++ b/libgnucash/engine/test/test-qofid.cpp @@ -0,0 +1,205 @@ +/******************************************************************** + * test-qofid.c: GLib g_test test suite for qofobject.c. * + * Copyright 2025 Stefan Koch * + * * + * This program is free software; you can redistribute it and/or * + * modify it under the terms of the GNU General Public License as * + * published by the Free Software Foundation; either version 2 of * + * the License, or (at your option) any later version. * + * * + * This program is distributed in the hope that it will be useful, * + * but WITHOUT ANY WARRANTY; without even the implied warranty of * + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * + * GNU General Public License for more details. * + * * + * You should have received a copy of the GNU General Public License* + * along with this program; if not, contact: * + * * + * Free Software Foundation Voice: +1-617-542-5942 * + * 51 Franklin Street, Fifth Floor Fax: +1-617-542-2652 * + * Boston, MA 02110-1301, USA gnu@gnu.org * +********************************************************************/ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wcpp" +#include +#pragma GCC diagnostic pop +#include +#include +#include + +#include "../qofid.h" +#include "../qofid-p.h" +#include "../qofbook.h" +#include "../gncJob.h" +#include "../qofinstance-p.h" + +// This is the error handler that does nothing but count how many times it is +// called. The handler_count is incremented every time. +static void +test_count_handler (const char *log_domain, GLogLevelFlags log_level, + const gchar *msg, void* user_data ) +{ + auto pint = (int*)user_data; + *pint += 1; +} + +TEST(QofIDTest, collection_new) +{ + auto col = qof_collection_new (QOF_ID_BOOK); + EXPECT_STREQ(qof_collection_get_type(col), QOF_ID_BOOK); + EXPECT_EQ(qof_collection_count(col), 0u); + + EXPECT_FALSE(qof_collection_is_dirty(col)); + qof_collection_mark_dirty(col); + EXPECT_TRUE(qof_collection_is_dirty(col)); + qof_collection_mark_clean(col); + EXPECT_FALSE(qof_collection_is_dirty(col)); + + EXPECT_EQ(qof_collection_get_data(col), nullptr); + int i = 0; + gpointer data = &i; + qof_collection_set_data(col, data); + EXPECT_EQ(qof_collection_get_data(col), data); + qof_collection_set_data(col, nullptr); + + qof_collection_destroy(col); +} + + +TEST(QofIDTest, collection_add) +{ + auto col = qof_collection_new(QOF_ID_BOOK); + auto book = qof_book_new(); + auto job = gncJobCreate(book); + gncJobBeginEdit(job); + + EXPECT_FALSE(qof_collection_add_entity(nullptr, nullptr)); + EXPECT_FALSE(qof_collection_add_entity(col, nullptr)); + EXPECT_FALSE(qof_collection_add_entity(nullptr, QOF_INSTANCE(book))); + + EXPECT_TRUE(qof_collection_add_entity(col, QOF_INSTANCE(book))); + EXPECT_FALSE(qof_collection_add_entity(col, QOF_INSTANCE(book))); + + auto old_guid = qof_instance_get_guid(QOF_INSTANCE(book)); + qof_instance_set_guid(QOF_INSTANCE(book), guid_null()); + EXPECT_FALSE(qof_collection_add_entity(col, QOF_INSTANCE(book))); + qof_instance_set_guid(QOF_INSTANCE(book), old_guid); + + + auto assert_count = 0; + auto oldlogger = g_log_set_default_handler(test_count_handler, &assert_count); + EXPECT_FALSE(qof_collection_add_entity(col, QOF_INSTANCE(job))); + EXPECT_EQ(assert_count, 1); + g_log_set_default_handler(oldlogger, nullptr); + + gncJobDestroy(job); + qof_collection_destroy(col); +} + +TEST(QofIDTest, collection_compare) +{ + auto target = qof_collection_new(QOF_ID_BOOK); + auto merge = qof_collection_new(QOF_ID_BOOK); + auto incompat = qof_collection_new(GNC_ID_JOB); + auto book1 = qof_book_new(); + auto book2 = qof_book_new(); + auto book3 = qof_book_new(); + auto book4 = qof_book_new(); + + // Special cases. + EXPECT_EQ(0, qof_collection_compare(nullptr, nullptr)); + EXPECT_EQ(0, qof_collection_compare(target, target)); + EXPECT_EQ(-1, qof_collection_compare(nullptr, merge)); + EXPECT_EQ(1, qof_collection_compare(target, nullptr)); + EXPECT_EQ(-1, qof_collection_compare(target, incompat)); + + EXPECT_EQ(0, qof_collection_compare(target, merge)); + qof_collection_add_entity(target, QOF_INSTANCE(book1)); + qof_collection_add_entity(merge, QOF_INSTANCE(book1)); + EXPECT_EQ(0, qof_collection_compare(target, merge)); + + qof_collection_add_entity(target, QOF_INSTANCE(book2)); + EXPECT_EQ(1, qof_collection_compare(target, merge)); + qof_collection_add_entity(merge, QOF_INSTANCE(book2)); + qof_collection_add_entity(merge, QOF_INSTANCE(book3)); + EXPECT_EQ(1, qof_collection_compare(target, merge)); + + auto old_guid = qof_instance_get_guid(QOF_INSTANCE(book3)); + qof_instance_set_guid(QOF_INSTANCE(book3), guid_null()); + EXPECT_EQ(-1, qof_collection_compare(target, merge)); + qof_instance_set_guid(QOF_INSTANCE(book3), old_guid); + + qof_collection_add_entity(target, QOF_INSTANCE(book4)); + old_guid = qof_instance_get_guid(QOF_INSTANCE(book4)); + qof_instance_set_guid(QOF_INSTANCE(book4), guid_null()); + EXPECT_EQ(-1, qof_collection_compare(target, merge)); + qof_instance_set_guid(QOF_INSTANCE(book4), old_guid); +} + +static void cb_visit(GncJob* job, std::vector* results_ptr) +{ + results_ptr->push_back(gncJobGetID(job)); +} + +static gint cb_compare(GncJob* job1, GncJob* job2) +{ + return g_strcmp0(gncJobGetID(job1), gncJobGetID(job2)); +} + +static bool is_in(const char* str, const std::vector str_vect) +{ + for(auto id : str_vect) + if(!g_strcmp0(id, str)) + return true; + return false; +} + +TEST(QofIDTest, collection_foreach) +{ + auto col = qof_collection_new(GNC_ID_JOB); + auto book = qof_book_new(); + auto job_ids = std::vector{"zzz", "ggg", "qqq", "aaa"}; + std::vector jobs; + for(auto id : job_ids) + { + auto job = gncJobCreate(book); + gncJobSetID(job, id); + jobs.push_back(job); + qof_collection_add_entity(col, QOF_INSTANCE(job)); + } + + std::vector results; + qof_collection_foreach(col, (QofInstanceForeachCB)&cb_visit, &results); + EXPECT_EQ(job_ids.size(), results.size()); + for( auto id : job_ids) + EXPECT_TRUE(is_in(id, results)); + + results.clear(); + qof_collection_foreach_sorted( + col, (QofInstanceForeachCB)&cb_visit, &results, (GCompareFunc)&cb_compare); + EXPECT_EQ(job_ids.size(), results.size()); + EXPECT_STREQ(results[0], "aaa"); + EXPECT_STREQ(results[1], "ggg"); + EXPECT_STREQ(results[2], "qqq"); + EXPECT_STREQ(results[3], "zzz"); +} + +TEST(QofIDTest, collection_print_dirty) +{ + auto col = qof_collection_new(QOF_ID_BOOK); + auto book = qof_book_new(); + qof_collection_add_entity(col, QOF_INSTANCE(book)); + + testing::internal::CaptureStdout(); + qof_collection_print_dirty(col, nullptr); + std::string output = testing::internal::GetCapturedStdout(); + EXPECT_STREQ("", output.c_str()); + + + qof_collection_mark_dirty(col); + testing::internal::CaptureStdout(); + qof_collection_print_dirty(col, nullptr); + output = testing::internal::GetCapturedStdout(); + EXPECT_EQ(output.find("Book collection is dirty."), size_t(0)); + qof_collection_mark_clean(col); +} From 0e3853bfb73cf0edf276ce98ac74523c230b2a38 Mon Sep 17 00:00:00 2001 From: Stefan Koch Date: Thu, 15 Jan 2026 07:10:20 -0500 Subject: [PATCH 4/7] Fix undefined behaviour in collection_compare_cb function. The collection_compare_cb function set the user_data pointer to point to a local variable of the function. That pointer is then used in the (only) calling function qof_collection_compare to make decisions. This is undefined. It is likely not an actual problem because the stack depth of the qof_collection_foreach followed by collection_compare_cb is deeper than the qof_collection_get_data call (and others in between if any) that the stack data user data stays uncorrupted. But, it is undefined behavior, and could cause really subtle bugs if these there are code changes that have deeper stack between the setting and using. Also, using this local variable is not necessary, the qof_collection_compare function already sets up a variable local to its scope for this that the collection_compare_cb can use directly. This commit removes the local (to collection_compare_cb) variable and uses the one setup in qof_collection_compare. The full coverage test for qofid.cpp passed before and after this change. --- libgnucash/engine/qofid.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/libgnucash/engine/qofid.cpp b/libgnucash/engine/qofid.cpp index b1bf7e8e50..3e47ff9158 100644 --- a/libgnucash/engine/qofid.cpp +++ b/libgnucash/engine/qofid.cpp @@ -139,7 +139,7 @@ collection_compare_cb (QofInstance *ent, gpointer user_data) QofCollection *target; QofInstance *e; const GncGUID *guid; - gint value; + gint* value; e = NULL; target = (QofCollection*)user_data; @@ -147,28 +147,25 @@ collection_compare_cb (QofInstance *ent, gpointer user_data) { return; } - value = *(gint*)qof_collection_get_data(target); - if (value != 0) + value = (gint*)qof_collection_get_data(target); + if (*value != 0) { return; } guid = qof_instance_get_guid(ent); if (guid_equal(guid, guid_null())) { - value = -1; - qof_collection_set_data(target, &value); + *value = -1; return; } g_return_if_fail (target->e_type == ent->e_type); e = qof_collection_lookup_entity(target, guid); if ( e == NULL ) { - value = 1; - qof_collection_set_data(target, &value); + *value = 1; return; } - value = 0; - qof_collection_set_data(target, &value); + *value = 0; } gint From be5933a1ec6dfc0c6d0211d62f330872c092d788 Mon Sep 17 00:00:00 2001 From: Stefan Koch Date: Thu, 15 Jan 2026 11:52:59 -0500 Subject: [PATCH 5/7] fixup: Remove memory leaks in the test code. --- libgnucash/engine/test/test-qofid.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libgnucash/engine/test/test-qofid.cpp b/libgnucash/engine/test/test-qofid.cpp index 9897a87eee..b9fbd4b76f 100644 --- a/libgnucash/engine/test/test-qofid.cpp +++ b/libgnucash/engine/test/test-qofid.cpp @@ -134,6 +134,10 @@ TEST(QofIDTest, collection_compare) qof_instance_set_guid(QOF_INSTANCE(book4), guid_null()); EXPECT_EQ(-1, qof_collection_compare(target, merge)); qof_instance_set_guid(QOF_INSTANCE(book4), old_guid); + + qof_collection_destroy(target); + qof_collection_destroy(merge); + qof_collection_destroy(incompat); } static void cb_visit(GncJob* job, std::vector* results_ptr) @@ -182,6 +186,8 @@ TEST(QofIDTest, collection_foreach) EXPECT_STREQ(results[1], "ggg"); EXPECT_STREQ(results[2], "qqq"); EXPECT_STREQ(results[3], "zzz"); + + qof_collection_destroy(col); } TEST(QofIDTest, collection_print_dirty) @@ -202,4 +208,5 @@ TEST(QofIDTest, collection_print_dirty) output = testing::internal::GetCapturedStdout(); EXPECT_EQ(output.find("Book collection is dirty."), size_t(0)); qof_collection_mark_clean(col); + qof_collection_destroy(col); } From 7f0d66d43977816de38b70554ad5555e6ab8fbe2 Mon Sep 17 00:00:00 2001 From: Stefan Koch Date: Thu, 15 Jan 2026 13:00:52 -0500 Subject: [PATCH 6/7] fixup: Add the new test-qofid.cpp file the the source distribution. --- libgnucash/engine/test/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/libgnucash/engine/test/CMakeLists.txt b/libgnucash/engine/test/CMakeLists.txt index 137ad90252..6d5b073694 100644 --- a/libgnucash/engine/test/CMakeLists.txt +++ b/libgnucash/engine/test/CMakeLists.txt @@ -242,6 +242,7 @@ set(test_engine_SOURCES_DIST test-numeric.cpp test-object.c test-qof.c + test-qofid.cpp test-qofbook.c test-qofinstance.cpp test-qofobject.c From 113af56272deac7deeb728c9a7caf5ad37190b9a Mon Sep 17 00:00:00 2001 From: Stefan Koch Date: Thu, 15 Jan 2026 13:14:36 -0500 Subject: [PATCH 7/7] fixup: Review comments about style of code. --- libgnucash/engine/test/test-qofid.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/libgnucash/engine/test/test-qofid.cpp b/libgnucash/engine/test/test-qofid.cpp index b9fbd4b76f..3e0b5cf34f 100644 --- a/libgnucash/engine/test/test-qofid.cpp +++ b/libgnucash/engine/test/test-qofid.cpp @@ -27,11 +27,11 @@ #include #include -#include "../qofid.h" -#include "../qofid-p.h" -#include "../qofbook.h" -#include "../gncJob.h" -#include "../qofinstance-p.h" +#include "qofid.h" +#include "qofid-p.h" +#include "qofbook.h" +#include "gncJob.h" +#include "qofinstance-p.h" // This is the error handler that does nothing but count how many times it is // called. The handler_count is incremented every time. @@ -150,19 +150,17 @@ static gint cb_compare(GncJob* job1, GncJob* job2) return g_strcmp0(gncJobGetID(job1), gncJobGetID(job2)); } -static bool is_in(const char* str, const std::vector str_vect) +static bool is_in(const char* str, const std::vector& str_vect) { - for(auto id : str_vect) - if(!g_strcmp0(id, str)) - return true; - return false; + return std::find_if(str_vect.begin(), str_vect.end(), + [str](auto id)->bool{ return !g_strcmp0(id, str); }) != str_vect.end(); } TEST(QofIDTest, collection_foreach) { auto col = qof_collection_new(GNC_ID_JOB); auto book = qof_book_new(); - auto job_ids = std::vector{"zzz", "ggg", "qqq", "aaa"}; + std::vector job_ids{"zzz", "ggg", "qqq", "aaa"}; std::vector jobs; for(auto id : job_ids) {