gnucash master: Multiple changes pushed
John Ralls
jralls at code.gnucash.org
Tue Jul 7 15:04:21 EDT 2015
Updated via https://github.com/Gnucash/gnucash/commit/6447be9b (commit)
via https://github.com/Gnucash/gnucash/commit/ca62782d (commit)
from https://github.com/Gnucash/gnucash/commit/919fe76c (commit)
commit 6447be9ba9c8ff513eaa8311d951c46366160e1a
Author: John Ralls <jralls at ceridwen.us>
Date: Tue Jul 7 12:04:06 2015 -0700
Document that KvpFrame and KvpValue take ownership of pointers passed to them.
diff --git a/src/libqof/qof/kvp-value.hpp b/src/libqof/qof/kvp-value.hpp
index e97fb36..7288ad0 100644
--- a/src/libqof/qof/kvp-value.hpp
+++ b/src/libqof/qof/kvp-value.hpp
@@ -41,7 +41,18 @@ extern "C"
* @{
*/
-/** Implements KvpValue using boost::variant.
+/** Implements KvpValue using boost::variant. Capable of holding the following
+ * types:
+ * * int64_t
+ * * double
+ * * gnc_numeric
+ * * const char*
+ * * GncGUID*
+ * * Timepsec
+ * * GList*
+ * * KvpFrame*
+ * * GDate
+ */
*/
struct KvpValueImpl
{
@@ -68,11 +79,23 @@ struct KvpValueImpl
KvpValueImpl& operator=(const KvpValueImpl&) noexcept;
/**
- * Move. The old object's datastore is set to int646_t 0.
+ * Move. The old object's datastore is set to int64_t 0.
*/
KvpValueImpl(KvpValueImpl && b) noexcept;
KvpValueImpl& operator=(KvpValueImpl && b) noexcept;
+ /** Create a KvpValue containing the passed in item. Note that for pointer
+ * types const char*, KvpFrame*, GncGUID*, and GList* the KvpValue takes
+ * ownership of the objcet and will delete/free it when the KvpValue is
+ * destroyed. That means these objects must be allocated in the free store
+ * or heap as follows:
+ * * const char*: GLib string allocation, e.g. g_strdup()/
+ * * KvpFrame*: operator new
+ * * GncGUID*: guid_new() or guid_copy()
+ * * GList*: Uses g_list_free(), so it's up to classes using this to empty
+ the list before destroying the KvpValue.
+ */
+
template <typename T>
KvpValueImpl(T) noexcept;
diff --git a/src/libqof/qof/kvp_frame.hpp b/src/libqof/qof/kvp_frame.hpp
index 949709f..52908cd 100644
--- a/src/libqof/qof/kvp_frame.hpp
+++ b/src/libqof/qof/kvp_frame.hpp
@@ -55,18 +55,33 @@
* they're part of and how they're used.
*
* ## Purpose
- * KVP is used to extend the class structure without directly reflecting the extension in the database or xML schema. The backend will directly load and store KVP slots without any checking, which allows older versions of GnuCash to load the database without complaint and without damaging the KVP data that they don't understand.
+ * KVP is used to extend the class structure without directly reflecting the
+ * extension in the database or xML schema. The backend will directly load and
+ * store KVP slots without any checking, which allows older versions of GnuCash
+ * to load the database without complaint and without damaging the KVP data that
+ * they don't understand.
*
- * When a feature is entirely implemented in KVP and doesn't affect the meaning of the books or other features, this isn't a problem, but when it's not true then it should be registered in @ref UtilFeature so that older versions of GnuCash will refuse to load the database.
+ * When a feature is entirely implemented in KVP and doesn't affect the meaning
+ * of the books or other features, this isn't a problem, but when it's not true
+ * then it should be registered in @ref UtilFeature so that older versions of
+ * GnuCash will refuse to load the database.
*
* ## Policy
* * Document every KVP slot in src/engine/kvp_doc.txt so that it is presented
* in @ref kvpvalues.
- * * Register a feature in @ref UtilFeature if the use of the KVP in any way affects the books or business computations.
- * * Key strings should be all lower case with '-', not spaces, separating words and '/' separating frames. Prefer longer and more descriptive names to abbreviations, and define a global const char[] to the key or path string so that the compiler will catch any typos.
- * * Make good use of the hierarchical nature of KVP by using frames to group related slots.
- * * Don't use the KVP API directly outside of libqof, and prefer to use the QofInstance and QofBook functions whenever feasible. If those functions aren't feasible write a class in libqof to abstract the use of KVP.
- * * Avoid re-using key names in different contexts (e.g. Transactions and Splits) unless the slot is used for the same purpose in both.
+ * * Register a feature in @ref UtilFeature if the use of the KVP in any way
+ affects the books or business computations.
+ * * Key strings should be all lower case with '-', not spaces, separating words
+ and '/' separating frames. Prefer longer and more descriptive names to
+ abbreviations, and define a global const char[] to the key or path string so
+ that the compiler will catch any typos.
+ * * Make good use of the hierarchical nature of KVP by using frames to group
+ related slots.
+ * * Don't use the KVP API directly outside of libqof, and prefer to use the
+ QofInstance and QofBook functions whenever feasible. If those functions
+ aren't feasible write a class in libqof to abstract the use of KVP.
+ * * Avoid re-using key names in different contexts (e.g. Transactions and
+ Splits) unless the slot is used for the same purpose in both.
* @{
*/
@@ -121,7 +136,9 @@ struct KvpFrameImpl
/**
* Set the value with the key in the immediate frame, replacing and
- * returning the old value if it exists or nullptr if it doesn't.
+ * returning the old value if it exists or nullptr if it doesn't. Takes
+ * ownership of new value and releases ownership of the returned old
+ * value. Values must be allocated on the free store with operator new.
* @param key: The key to insert/replace.
* @param newvalue: The value to set at key.
* @return The old value if there was one or nullptr.
@@ -130,7 +147,10 @@ struct KvpFrameImpl
/**
* Set the value with the key in a subframe following the keys in path,
* replacing and returning the old value if it exists or nullptr if it
- * doesn't.
+ * doesn't. Takes ownership of new value and releases ownership of the
+ * returned old value. Values must be allocated on the free store with
+ * operator new.
+ * @param key: The key to insert/replace.
* @throw invalid_argument if the path doesn't exist.
* @param path: The path of subframes leading to the frame in which to
* insert/replace.
@@ -141,21 +161,21 @@ struct KvpFrameImpl
/**
* Set the value with the key in a subframe following the keys in path,
* replacing and returning the old value if it exists or nullptr if it
- * doesn't. Creates any missing intermediate frames.
+ * doesn't. Creates any missing intermediate frames. Takes
+ * ownership of new value and releases ownership of the returned old
+ * value. Values must be allocated on the free store with operator new.
* @param path: The path of subframes as a '/'-delimited string leading to
* the frame in which to insert/replace.
* @param newvalue: The value to set at key.
* @return The old value if there was one or nullptr.
*/
KvpValue* set_path(const char* path, KvpValue* newvalue) noexcept;
- /**
- * Make a string representation of the frame. Mostly useful for debugging.
- * @return A std::string representing the frame and all its children.
- */
- /**
+ /**
* Set the value with the key in a subframe following the keys in path,
* replacing and returning the old value if it exists or nullptr if it
- * doesn't. Creates any missing intermediate frames.
+ * doesn't. Creates any missing intermediate frames.Takes
+ * ownership of new value and releases ownership of the returned old
+ * value. Values must be allocated on the free store with operator new.
* @param path: The path of subframes as a std::vector leading to the
* frame in which to insert/replace.
* @param newvalue: The value to set at key.
commit ca62782d93805206a2ae053b9a078b80c88cc594
Author: John Ralls <jralls at ceridwen.us>
Date: Tue Jul 7 12:03:37 2015 -0700
Fix up double-frees or frees of unallocated objects in KVP.
Revealed by ensuring that KvpFrame and KvpValue deep-destroy their contents.
diff --git a/src/backend/dbi/test/test-backend-dbi-basic.cpp b/src/backend/dbi/test/test-backend-dbi-basic.cpp
index cd00f22..4ea5418 100644
--- a/src/backend/dbi/test/test-backend-dbi-basic.cpp
+++ b/src/backend/dbi/test/test-backend-dbi-basic.cpp
@@ -109,7 +109,7 @@ setup_memory (Fixture *fixture, gconstpointer pData)
frame->set("string-val", new KvpValue("abcdefghijklmnop"));
auto guid = qof_instance_get_guid (QOF_INSTANCE(acct1));
- frame->set("guid-val", new KvpValue(const_cast<GncGUID*>(guid)));
+ frame->set("guid-val", new KvpValue(const_cast<GncGUID*>(guid_copy(guid))));
gnc_account_append_child (root, acct1);
diff --git a/src/backend/xml/test/test-kvp-frames.cpp b/src/backend/xml/test/test-kvp-frames.cpp
index 5fd4c8f..dca51d1 100644
--- a/src/backend/xml/test/test-kvp-frames.cpp
+++ b/src/backend/xml/test/test-kvp-frames.cpp
@@ -120,7 +120,6 @@ test_kvp_frames1(void)
test_kvp_copy_compare(i, test_frame1, test_val1, test_key);
test_kvp_copy_get_slot(i, test_frame1, test_val1, test_key);
- delete test_val1;
g_free(test_key);
delete test_frame1;
}
diff --git a/src/engine/Split.c b/src/engine/Split.c
index a8d5f17..8d6142d 100644
--- a/src/engine/Split.c
+++ b/src/engine/Split.c
@@ -2051,7 +2051,7 @@ xaccSplitAddPeerSplit (Split *split, const Split *other_split,
guid = qof_instance_get_guid (QOF_INSTANCE (other_split));
xaccTransBeginEdit (split->parent);
qof_instance_kvp_add_guid (QOF_INSTANCE (split), "lot-split",
- timespec_now(), "peer_guid", guid);
+ timespec_now(), "peer_guid", guid_copy(guid));
mark_split (split);
qof_instance_set_dirty (QOF_INSTANCE (split));
xaccTransCommitEdit (split->parent);
diff --git a/src/engine/test/utest-Split.cpp b/src/engine/test/utest-Split.cpp
index d692b6a..aa201ef 100644
--- a/src/engine/test/utest-Split.cpp
+++ b/src/engine/test/utest-Split.cpp
@@ -319,7 +319,7 @@ test_xaccDupeSplit (Fixture *fixture, gconstpointer pData)
}
/* xaccSplitCloneNoKvp
Split *
-xaccSplitCloneNoKvp (const Split *s)// C: 1
+xaccSplitCloneNoKvp (const Split *s)// C: 1
*/
static void
test_xaccSplitCloneNoKvp (Fixture *fixture, gconstpointer pData)
@@ -743,7 +743,7 @@ test_xaccSplitDetermineGainStatus (Fixture *fixture, gconstpointer pData)
g_assert (fixture->split->gains_split == NULL);
g_assert_cmpint (fixture->split->gains, ==, GAINS_STATUS_A_VDIRTY | GAINS_STATUS_DATE_DIRTY);
- fixture->split->inst.kvp_data->set("gains-source", new KvpValue(const_cast<GncGUID*>(g_guid)));
+ fixture->split->inst.kvp_data->set("gains-source", new KvpValue(const_cast<GncGUID*>(guid_copy(g_guid))));
g_assert (fixture->split->gains_split == NULL);
fixture->split->gains = GAINS_STATUS_UNKNOWN;
xaccSplitDetermineGainStatus (fixture->split);
diff --git a/src/libqof/qof/test/test-kvp-frame.cpp b/src/libqof/qof/test/test-kvp-frame.cpp
index 4f30dd8..956f38b 100644
--- a/src/libqof/qof/test/test-kvp-frame.cpp
+++ b/src/libqof/qof/test/test-kvp-frame.cpp
@@ -179,6 +179,7 @@ TEST_F (KvpFrameTest, GetSlotPath)
EXPECT_EQ (nullptr, t_root.set(path2, v2));
EXPECT_EQ (v1, t_root.get_slot(path1));
EXPECT_EQ (nullptr, t_root.get_slot(path2));
+ t_root.set_path(path1, nullptr);
t_root.set_path(path3, v1);
EXPECT_EQ (v1, t_root.get_slot(path3a));
}
Summary of changes:
src/backend/dbi/test/test-backend-dbi-basic.cpp | 2 +-
src/backend/xml/test/test-kvp-frames.cpp | 1 -
src/engine/Split.c | 2 +-
src/engine/test/utest-Split.cpp | 4 +-
src/libqof/qof/kvp-value.hpp | 27 ++++++++++++-
src/libqof/qof/kvp_frame.hpp | 52 +++++++++++++++++--------
src/libqof/qof/test/test-kvp-frame.cpp | 1 +
7 files changed, 66 insertions(+), 23 deletions(-)
More information about the gnucash-changes
mailing list