gnucash maint: Multiple changes pushed
John Ralls
jralls at code.gnucash.org
Mon Feb 27 18:05:32 EST 2023
Updated via https://github.com/Gnucash/gnucash/commit/8a7c5392 (commit)
via https://github.com/Gnucash/gnucash/commit/ddc3f288 (commit)
from https://github.com/Gnucash/gnucash/commit/de9c0eb5 (commit)
commit 8a7c53925870501c8f103c8f09961ecba3341c84
Author: John Ralls <jralls at ceridwen.us>
Date: Mon Feb 27 14:54:46 2023 -0800
Bug 798748 - Transaction Notes field's value does not appear in...
reverse transaction.
The proximate cause was that xaccTransBeginEdit put the KVP cache
variable of the new transaction in a state that prevented the value
from being copied. More generally the KVP cache variables didn't
handle any invalidating events.
With the change to GValue usage in qof_instance_get_kvp it's now
a simple memory dereference with no copying except for POD types
so caching is no longer useful. This commit removes caching from
Transaction, eliminating the notes problem.
diff --git a/libgnucash/engine/Transaction.c b/libgnucash/engine/Transaction.c
index 4b1888cc3..987706f74 100644
--- a/libgnucash/engine/Transaction.c
+++ b/libgnucash/engine/Transaction.c
@@ -255,9 +255,6 @@ void gen_event_trans (Transaction *trans)
}
}
-static const char*
-is_unset = "unset";
-
/* GObject Initialization */
G_DEFINE_TYPE(Transaction, gnc_transaction, QOF_TYPE_INSTANCE)
@@ -274,11 +271,6 @@ gnc_transaction_init(Transaction* trans)
trans->date_posted = 0;
trans->marker = 0;
trans->orig = NULL;
- trans->readonly_reason = (char*) is_unset;
- trans->isClosingTxn_cached = -1;
- trans->notes = (char*) is_unset;
- trans->doclink = (char*) is_unset;
- trans->void_reason = (char*) is_unset;
trans->txn_type = TXN_TYPE_UNCACHED;
LEAVE (" ");
}
@@ -821,24 +813,12 @@ xaccFreeTransaction (Transaction *trans)
/* free up transaction strings */
CACHE_REMOVE(trans->num);
CACHE_REMOVE(trans->description);
- if (trans->readonly_reason != is_unset)
- g_free (trans->readonly_reason);
- if (trans->doclink != is_unset)
- g_free (trans->doclink);
- if (trans->void_reason != is_unset)
- g_free (trans->void_reason);
- if (trans->notes != is_unset)
- g_free (trans->notes);
/* Just in case someone looks up freed memory ... */
trans->num = (char *) 1;
trans->description = NULL;
trans->date_entered = 0;
trans->date_posted = 0;
- trans->readonly_reason = NULL;
- trans->doclink = NULL;
- trans->notes = NULL;
- trans->void_reason = NULL;
if (trans->orig)
{
xaccFreeTransaction (trans->orig);
@@ -2047,7 +2027,7 @@ xaccTransSetDatePostedGDate (Transaction *trans, GDate date)
* the future a date which was set as *date* (without time) can
* clearly be distinguished from the time64. */
g_value_init (&v, G_TYPE_DATE);
- g_value_set_boxed (&v, &date);
+ g_value_set_static_boxed (&v, &date);
qof_instance_set_kvp (QOF_INSTANCE(trans), &v, 1, TRANS_DATE_POSTED);
g_value_unset (&v);
/* mark dirty and commit handled by SetDateInternal */
@@ -2105,7 +2085,7 @@ xaccTransSetDateDue (Transaction * trans, time64 time)
GValue v = G_VALUE_INIT;
if (!trans) return;
g_value_init (&v, GNC_TYPE_TIME64);
- g_value_set_boxed (&v, &time);
+ g_value_set_static_boxed (&v, &time);
xaccTransBeginEdit(trans);
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, TRANS_DATE_DUE_KVP);
qof_instance_set_dirty(QOF_INSTANCE(trans));
@@ -2126,7 +2106,7 @@ xaccTransSetTxnType (Transaction *trans, char type)
g_value_unset (&v);
return;
}
- g_value_set_string (&v, s);
+ g_value_set_static_string (&v, s);
xaccTransBeginEdit(trans);
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, TRANS_TXN_TYPE_KVP);
qof_instance_set_dirty(QOF_INSTANCE(trans));
@@ -2142,10 +2122,6 @@ void xaccTransClearReadOnly (Transaction *trans)
qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, TRANS_READ_ONLY_REASON);
qof_instance_set_dirty(QOF_INSTANCE(trans));
xaccTransCommitEdit(trans);
-
- if (trans->readonly_reason != is_unset)
- g_free (trans->readonly_reason);
- trans->readonly_reason = NULL;
}
}
@@ -2156,16 +2132,12 @@ xaccTransSetReadOnly (Transaction *trans, const char *reason)
{
GValue v = G_VALUE_INIT;
g_value_init (&v, G_TYPE_STRING);
- g_value_set_string (&v, reason);
+ g_value_set_static_string (&v, reason);
xaccTransBeginEdit(trans);
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, TRANS_READ_ONLY_REASON);
qof_instance_set_dirty(QOF_INSTANCE(trans));
g_value_unset (&v);
xaccTransCommitEdit(trans);
-
- if (trans->readonly_reason != is_unset)
- g_free (trans->readonly_reason);
- trans->readonly_reason = g_strdup (reason);
}
}
@@ -2218,25 +2190,16 @@ xaccTransSetDocLink (Transaction *trans, const char *doclink)
{
if (!trans || !doclink) return;
- if (trans->doclink != is_unset)
- {
- if (!g_strcmp0 (doclink, trans->doclink))
- return;
-
- g_free (trans->doclink);
- }
xaccTransBeginEdit(trans);
if (doclink[0] == '\0')
{
- trans->doclink = NULL;
qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, doclink_uri_str);
}
else
{
GValue v = G_VALUE_INIT;
- trans->doclink = g_strdup (doclink);
g_value_init (&v, G_TYPE_STRING);
- g_value_set_string (&v, doclink);
+ g_value_set_static_string (&v, doclink); //Gets copied at the other end
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, doclink_uri_str);
g_value_unset (&v);
}
@@ -2257,18 +2220,9 @@ xaccTransSetNotes (Transaction *trans, const char *notes)
{
GValue v = G_VALUE_INIT;
if (!trans || !notes) return;
- if (trans->notes != is_unset)
- {
- if (!g_strcmp0 (notes, trans->notes))
- return;
-
- g_free (trans->notes);
- }
g_value_init (&v, G_TYPE_STRING);
- g_value_set_string (&v, notes);
+ g_value_set_static_string (&v, notes);
xaccTransBeginEdit(trans);
-
- trans->notes = g_strdup (notes);
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, trans_notes_str);
qof_instance_set_dirty(QOF_INSTANCE(trans));
g_value_unset (&v);
@@ -2288,12 +2242,10 @@ xaccTransSetIsClosingTxn (Transaction *trans, gboolean is_closing)
g_value_set_int64 (&v, 1);
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, trans_is_closing_str);
g_value_unset (&v);
- trans->isClosingTxn_cached = 1;
}
else
{
qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, trans_is_closing_str);
- trans->isClosingTxn_cached = 0;
}
qof_instance_set_dirty(QOF_INSTANCE(trans));
xaccTransCommitEdit(trans);
@@ -2431,50 +2383,46 @@ const char *
xaccTransGetDocLink (const Transaction *trans)
{
g_return_val_if_fail (trans, NULL);
- if (trans->doclink == is_unset)
- {
- GValue v = G_VALUE_INIT;
- Transaction *t = (Transaction*) trans;
- qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, doclink_uri_str);
- t->doclink = G_VALUE_HOLDS_STRING (&v) ? g_value_dup_string (&v) : NULL;
- g_value_unset (&v);
- }
- return trans->doclink;
+
+ GValue v = G_VALUE_INIT;
+ Transaction *t = (Transaction*) trans;
+ qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, doclink_uri_str);
+ const char* doclink = G_VALUE_HOLDS_STRING (&v) ? g_value_get_string (&v) : NULL;
+ g_value_unset (&v);
+
+ return doclink;
}
const char *
xaccTransGetNotes (const Transaction *trans)
{
g_return_val_if_fail (trans, NULL);
- if (trans->notes == is_unset)
- {
- GValue v = G_VALUE_INIT;
- Transaction *t = (Transaction*) trans;
- qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, trans_notes_str);
- t->notes = G_VALUE_HOLDS_STRING (&v) ? g_value_dup_string (&v) : NULL;
- g_value_unset (&v);
- }
- return trans->notes;
+
+ GValue v = G_VALUE_INIT;
+ Transaction *t = (Transaction*) trans;
+ qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, trans_notes_str);
+ const char *notes = G_VALUE_HOLDS_STRING (&v) ? g_value_dup_string (&v) : NULL;
+ g_value_unset (&v);
+
+ return notes;
}
gboolean
xaccTransGetIsClosingTxn (const Transaction *trans)
{
if (!trans) return FALSE;
- if (trans->isClosingTxn_cached == -1)
- {
- Transaction* trans_nonconst = (Transaction*) trans;
- GValue v = G_VALUE_INIT;
- qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, trans_is_closing_str);
- if (G_VALUE_HOLDS_INT64 (&v))
- trans_nonconst->isClosingTxn_cached = (g_value_get_int64 (&v) ? 1 : 0);
- else
- trans_nonconst->isClosingTxn_cached = 0;
- g_value_unset (&v);
- }
- return (trans->isClosingTxn_cached == 1)
- ? TRUE
- : FALSE;
+
+ Transaction* trans_nonconst = (Transaction*) trans;
+ GValue v = G_VALUE_INIT;
+ gboolean rv;
+ qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, trans_is_closing_str);
+ if (G_VALUE_HOLDS_INT64 (&v))
+ rv = (g_value_get_int64 (&v) ? 1 : 0);
+ else
+ rv = 0;
+ g_value_unset (&v);
+
+ return rv;
}
/********************************************************************\
@@ -2604,15 +2552,12 @@ xaccTransGetReadOnly (Transaction *trans)
if (!trans)
return NULL;
- if (trans->readonly_reason == is_unset)
- {
- GValue v = G_VALUE_INIT;
- qof_instance_get_kvp (QOF_INSTANCE(trans), &v, 1, TRANS_READ_ONLY_REASON);
- trans->readonly_reason = G_VALUE_HOLDS_STRING (&v) ?
- g_value_dup_string (&v) : NULL;
- g_value_unset (&v);
- }
- return trans->readonly_reason;
+ GValue v = G_VALUE_INIT;
+ qof_instance_get_kvp (QOF_INSTANCE(trans), &v, 1, TRANS_READ_ONLY_REASON);
+ const char *readonly_reason = G_VALUE_HOLDS_STRING (&v) ?
+ g_value_get_string (&v) : NULL;
+ g_value_unset (&v);
+ return readonly_reason;
}
static gboolean
@@ -2810,16 +2755,13 @@ xaccTransVoid(Transaction *trans, const char *reason)
else
g_value_init (&v, G_TYPE_STRING);
- g_value_set_string (&v, _("Voided transaction"));
+ g_value_set_static_string (&v, _("Voided transaction"));
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, trans_notes_str);
- g_value_set_string (&v, reason);
+ g_value_set_static_string (&v, reason);
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, void_reason_str);
- if (trans->void_reason != is_unset)
- g_free (trans->void_reason);
- trans->void_reason = g_strdup (reason);
gnc_time64_to_iso8601_buff (gnc_time(NULL), iso8601_str);
- g_value_set_string (&v, iso8601_str);
+ g_value_set_static_string (&v, iso8601_str);
qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, void_time_str);
g_value_unset (&v);
@@ -2841,15 +2783,13 @@ const char *
xaccTransGetVoidReason(const Transaction *trans)
{
g_return_val_if_fail (trans, NULL);
- if (trans->void_reason == is_unset)
- {
- GValue v = G_VALUE_INIT;
- Transaction *t = (Transaction*) trans;
- qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, void_reason_str);
- t->void_reason = G_VALUE_HOLDS_STRING (&v) ? g_value_dup_string (&v) : NULL;
- g_value_unset (&v);
- }
- return trans->void_reason;
+
+ GValue v = G_VALUE_INIT;
+ qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, void_reason_str);
+ const char *void_reason = G_VALUE_HOLDS_STRING (&v) ? g_value_get_string (&v) : NULL;
+ g_value_unset (&v);
+
+ return void_reason;
}
time64
@@ -2889,8 +2829,6 @@ xaccTransUnvoid (Transaction *trans)
qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, void_reason_str);
qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, void_time_str);
g_value_unset (&v);
- g_free (trans->void_reason);
- trans->void_reason = NULL;
FOR_EACH_SPLIT(trans, xaccSplitUnvoid(s));
@@ -2927,7 +2865,7 @@ xaccTransReverse (Transaction *orig)
/* Now update the original with a pointer to the new one */
g_value_init (&v, GNC_TYPE_GUID);
- g_value_set_boxed (&v, xaccTransGetGUID(trans));
+ g_value_set_static_boxed (&v, xaccTransGetGUID(trans));
qof_instance_set_kvp (QOF_INSTANCE (orig), &v, 1, TRANS_REVERSED_BY);
g_value_unset (&v);
diff --git a/libgnucash/engine/TransactionP.h b/libgnucash/engine/TransactionP.h
index 3e107ff39..0a22be0cc 100644
--- a/libgnucash/engine/TransactionP.h
+++ b/libgnucash/engine/TransactionP.h
@@ -111,23 +111,11 @@ struct transaction_s
*/
Transaction *orig;
- /* The readonly_reason is a string that indicates why a transaction
- * is marked as read-only. If NULL, the transaction is read-write.
- * This value is stored in kvp, but we cache a copy here for
- * performance reasons.
+ /* A flag to indicate when a transaction represents an invoice, a payment,
+ * or a link between the two.
*/
- char * readonly_reason;
-
- char * doclink;
- char * void_reason;
- char * notes;
-
char txn_type;
- /* Cached bool value to indicate whether this is a closing txn. This is
- * cached from the KVP value because it is queried a lot. Tri-state value: -1
- * = uninitialized; 0 = FALSE, 1 = TRUE. */
- gint isClosingTxn_cached;
};
struct _TransactionClass
diff --git a/libgnucash/engine/mocks/gmock-Transaction.h b/libgnucash/engine/mocks/gmock-Transaction.h
index ce82437fa..e9dcd53fc 100644
--- a/libgnucash/engine/mocks/gmock-Transaction.h
+++ b/libgnucash/engine/mocks/gmock-Transaction.h
@@ -32,8 +32,6 @@ public:
date_posted = 0;
marker = 0;
orig = nullptr;
- readonly_reason = nullptr;
- isClosingTxn_cached = -1;
}
void* operator new(size_t size)
{
commit ddc3f28899c71579abd9b2bd822baa6b95fc41d6
Author: John Ralls <jralls at ceridwen.us>
Date: Mon Feb 27 13:02:27 2023 -0800
[kvp] Use static strings and boxed in gvalue_from_kvp_value.
Saves allocating and copying complex values, avoiding potential
memory leaks.
diff --git a/libgnucash/engine/kvp-frame.cpp b/libgnucash/engine/kvp-frame.cpp
index 56be042d4..c21370d05 100644
--- a/libgnucash/engine/kvp-frame.cpp
+++ b/libgnucash/engine/kvp-frame.cpp
@@ -261,15 +261,17 @@ kvp_value_list_from_gvalue (GValue *gval, gpointer pList)
}
GValue*
-gvalue_from_kvp_value (const KvpValue *kval)
+gvalue_from_kvp_value (const KvpValue *kval, GValue* val)
{
- GValue *val;
gnc_numeric num;
Time64 tm;
GDate gdate;
if (kval == NULL) return NULL;
- val = g_slice_new0 (GValue);
+ if (!val)
+ val = g_slice_new0 (GValue);
+ else
+ g_value_unset(val);
switch (kval->get_type())
{
@@ -283,36 +285,28 @@ gvalue_from_kvp_value (const KvpValue *kval)
break;
case KvpValue::Type::NUMERIC:
g_value_init (val, GNC_TYPE_NUMERIC);
- num = kval->get<gnc_numeric>();
- g_value_set_boxed (val, &num);
+ g_value_set_static_boxed (val, kval->get_ptr<gnc_numeric>());
break;
case KvpValue::Type::STRING:
g_value_init (val, G_TYPE_STRING);
- g_value_set_string (val, kval->get<const char*>());
+ g_value_set_static_string (val, kval->get<const char*>());
break;
case KvpValue::Type::GUID:
g_value_init (val, GNC_TYPE_GUID);
- g_value_set_boxed (val, kval->get<GncGUID*>());
+ g_value_set_static_boxed (val, kval->get<GncGUID*>());
break;
case KvpValue::Type::TIME64:
g_value_init (val, GNC_TYPE_TIME64);
- tm = kval->get<Time64>();
- g_value_set_boxed (val, &tm);
+ g_value_set_boxed (val, kval->get_ptr<Time64>());
break;
case KvpValue::Type::GDATE:
g_value_init (val, G_TYPE_DATE);
- gdate = kval->get<GDate>();
- g_value_set_boxed (val, &gdate);
+ g_value_set_static_boxed (val, kval->get_ptr<GDate>());
break;
case KvpValue::Type::GLIST:
{
- GList *gvalue_list = NULL;
- GList *kvp_list = kval->get<GList*>();
- g_list_foreach (kvp_list, (GFunc)gvalue_list_from_kvp_value,
- &gvalue_list);
g_value_init (val, GNC_TYPE_VALUE_LIST);
- gvalue_list = g_list_reverse (gvalue_list);
- g_value_set_boxed (val, gvalue_list);
+ g_value_set_static_boxed (val, kval->get<GList*>());
break;
}
/* No transfer of KVP frames outside of QofInstance-derived classes! */
diff --git a/libgnucash/engine/kvp-value.hpp b/libgnucash/engine/kvp-value.hpp
index 61b7dfd99..e7083b2f2 100644
--- a/libgnucash/engine/kvp-value.hpp
+++ b/libgnucash/engine/kvp-value.hpp
@@ -142,6 +142,8 @@ struct KvpValueImpl
template <typename T>
T get() const noexcept;
+ template <typename T>
+ const T* get_ptr() const noexcept;
template <typename T>
void set(T) noexcept;
@@ -178,6 +180,13 @@ KvpValueImpl::get() const noexcept
return boost::get<T>(datastore);
}
+template <typename T> const T*
+KvpValueImpl::get_ptr() const noexcept
+{
+ if (this->datastore.type() != typeid(T)) return nullptr;
+ return boost::get<T>(&datastore);
+}
+
template <typename T> void
KvpValueImpl::set(T val) noexcept
{
@@ -190,7 +199,7 @@ KvpValueImpl::set(T val) noexcept
* @param kval: A KvpValue.
* @return GValue*. Must be freed with g_free().
*/
-GValue* gvalue_from_kvp_value (const KvpValue *kval);
+GValue* gvalue_from_kvp_value (const KvpValue *kval, GValue* val = nullptr);
/** Convert a gvalue into a kvpvalue.
* @param gval: A GValue of a type KvpValue can digest.
diff --git a/libgnucash/engine/qofinstance.cpp b/libgnucash/engine/qofinstance.cpp
index 16b964b7e..97c3063e4 100644
--- a/libgnucash/engine/qofinstance.cpp
+++ b/libgnucash/engine/qofinstance.cpp
@@ -1090,15 +1090,7 @@ qof_instance_get_kvp (QofInstance * inst, GValue * value, unsigned count, ...)
for (unsigned i{0}; i < count; ++i)
path.push_back (va_arg (args, char const *));
va_end (args);
- auto temp = gvalue_from_kvp_value (inst->kvp_data->get_slot (path));
- if (G_IS_VALUE (temp))
- {
- if (G_IS_VALUE (value))
- g_value_unset (value);
- g_value_init (value, G_VALUE_TYPE (temp));
- g_value_copy (temp, value);
- gnc_gvalue_free (temp);
- }
+ gvalue_from_kvp_value (inst->kvp_data->get_slot (path), value);
}
void
Summary of changes:
libgnucash/engine/Transaction.c | 164 +++++++++-------------------
libgnucash/engine/TransactionP.h | 16 +--
libgnucash/engine/kvp-frame.cpp | 28 ++---
libgnucash/engine/kvp-value.hpp | 11 +-
libgnucash/engine/mocks/gmock-Transaction.h | 2 -
libgnucash/engine/qofinstance.cpp | 10 +-
6 files changed, 75 insertions(+), 156 deletions(-)
More information about the gnucash-changes
mailing list