diff --git a/src/backend/xml/io-gncxml-v1.cpp b/src/backend/xml/io-gncxml-v1.cpp index 29fcad6c06..3cfbc9f614 100644 --- a/src/backend/xml/io-gncxml-v1.cpp +++ b/src/backend/xml/io-gncxml-v1.cpp @@ -806,7 +806,7 @@ kvp_frame_slot_end_handler(gpointer data_for_children, if (key_node_count != 1) return(FALSE); value_cr->should_cleanup = TRUE; - f->replace_nc(key, value); + f->set(key, value); if (delete_value) delete value; return(TRUE); diff --git a/src/backend/xml/sixtp-dom-parsers.cpp b/src/backend/xml/sixtp-dom-parsers.cpp index 71a92adb82..49f6311889 100644 --- a/src/backend/xml/sixtp-dom-parsers.cpp +++ b/src/backend/xml/sixtp-dom-parsers.cpp @@ -446,7 +446,7 @@ dom_tree_to_kvp_frame_given(xmlNodePtr node, KvpFrame *frame) if (val) { //We're deleting the old KvpValue returned by replace_nc(). - delete frame->replace_nc(key, val); + delete frame->set(key, val); } else { diff --git a/src/libqof/qof/kvp-value.cpp b/src/libqof/qof/kvp-value.cpp index d24c6be06c..ce79dfc310 100644 --- a/src/libqof/qof/kvp-value.cpp +++ b/src/libqof/qof/kvp-value.cpp @@ -325,7 +325,7 @@ delete_visitor::operator()(GncGUID * & value) template <> void delete_visitor::operator()(KvpFrame * & value) { - kvp_frame_delete(value); + delete value; } KvpValueImpl::~KvpValueImpl() noexcept diff --git a/src/libqof/qof/kvp_frame.cpp b/src/libqof/qof/kvp_frame.cpp index b60ff162f5..e3246c0922 100644 --- a/src/libqof/qof/kvp_frame.cpp +++ b/src/libqof/qof/kvp_frame.cpp @@ -51,11 +51,12 @@ KvpFrameImpl::KvpFrameImpl(const KvpFrameImpl & rhs) noexcept ); } -KvpValueImpl * KvpFrameImpl::replace_nc(const char * key, KvpValueImpl * value) noexcept +KvpValue* +KvpFrameImpl::set(const char* key, KvpValue* value) noexcept { if (!key) return nullptr; auto spot = m_valuemap.find(key); - KvpValueImpl * ret {nullptr}; + KvpValue* ret {nullptr}; if (spot != m_valuemap.end()) { qof_string_cache_remove(spot->first); @@ -72,6 +73,53 @@ KvpValueImpl * KvpFrameImpl::replace_nc(const char * key, KvpValueImpl * value) return ret; } +static inline KvpFrameImpl* +walk_path_or_nullptr(const KvpFrameImpl* frame, Path& path) +{ + KvpFrameImpl* cur_frame = const_cast(frame); + for(auto key:path) + { + auto slot = cur_frame->get_slot(key); + if (slot == nullptr || slot->get_type() != KVP_TYPE_FRAME) + return nullptr; + cur_frame = slot->get(); + } + return cur_frame; +} + + +KvpValue* +KvpFrameImpl::set(Path path, KvpValue* value) noexcept +{ + const char* last_key = path.back(); + path.pop_back(); + auto cur_frame = walk_path_or_nullptr(this, path); + if (cur_frame == nullptr) + return nullptr; + return cur_frame->set(last_key, value); +} + +KvpValue* +KvpFrameImpl::set_path(Path path, KvpValue* value) noexcept +{ + auto cur_frame = this; + const char* last_key = path.back(); + path.pop_back(); + for(auto key:path) + { + auto slot = cur_frame->get_slot(key); + if (slot == nullptr || slot->get_type() != KVP_TYPE_FRAME) + { + auto new_frame = new KvpFrame; + delete cur_frame->set(key, new KvpValue{new_frame}); + cur_frame = new_frame; + continue; + } + cur_frame = slot->get(); + } + return cur_frame->set(last_key, value); +} + std::string KvpFrameImpl::to_string() const noexcept { @@ -131,6 +179,18 @@ KvpFrameImpl::get_slot(const char * key) const noexcept return spot->second; } +KvpValueImpl * +KvpFrameImpl::get_slot(Path path) const noexcept +{ + const char* last_key = path.back(); + path.pop_back(); + auto cur_frame = walk_path_or_nullptr(this, path); + if (cur_frame == nullptr) + return nullptr; + return cur_frame->get_slot(last_key); + +} + int compare(const KvpFrameImpl * one, const KvpFrameImpl * two) noexcept { if (one && !two) return 1; @@ -205,9 +265,7 @@ kvp_frame_get_keys(const KvpFrame * frame) gboolean kvp_frame_is_empty(const KvpFrame * frame) { - if (!frame) return TRUE; - auto realframe = static_cast(frame); - return realframe->get_keys().size() == 0; + return frame->empty(); } KvpFrame * @@ -222,26 +280,20 @@ kvp_frame_copy(const KvpFrame * frame) * removing the key from the KVP tree. */ static KvpValue * -kvp_frame_replace_slot_nc (KvpFrame * frame, const char * slot, - KvpValue * new_value) +kvp_frame_replace_slot_nc (KvpFrame* frame, const char* slot, KvpValue* value) { - if (!frame) return NULL; - - auto realframe = static_cast(frame); - auto realnewvalue = static_cast(new_value); - return realframe->replace_nc(slot, realnewvalue); + if (!frame) return nullptr; + return frame->set(slot, value); } /* Passing in a null value into this routine has the effect * of deleting the old value stored at this slot. */ static inline void -kvp_frame_set_slot_destructively(KvpFrame * frame, const char * slot, - KvpValue * new_value) +kvp_frame_set_slot_destructively(KvpFrame* frame, const char* slot, + KvpValue* value) { - KvpValue * old_value; - old_value = kvp_frame_replace_slot_nc (frame, slot, new_value); - kvp_value_delete (old_value); + delete frame->set(slot, value); } /* ============================================================ */ @@ -584,14 +636,12 @@ void kvp_frame_set_slot(KvpFrame * frame, const char * slot, KvpValue * value) { - KvpValue *new_value = NULL; - + KvpValue* new_value{nullptr}; if (!frame) return; - g_return_if_fail (slot && *slot != '\0'); if (value) new_value = kvp_value_copy(value); - kvp_frame_set_slot_destructively(frame, slot, new_value); + delete frame->set(slot, new_value); } void @@ -599,18 +649,16 @@ kvp_frame_set_slot_nc(KvpFrame * frame, const char * slot, KvpValue * value) { if (!frame) return; - g_return_if_fail (slot && *slot != '\0'); - kvp_frame_set_slot_destructively(frame, slot, value); + frame->set(slot, value); } KvpValue * kvp_frame_get_slot(const KvpFrame * frame, const char * slot) { - if (!frame) return NULL; - auto realframe = static_cast(frame); - return realframe->get_slot(slot); + if (!frame) return nullptr; + return frame->get_slot(slot); } void diff --git a/src/libqof/qof/kvp_frame.h b/src/libqof/qof/kvp_frame.h index 7de7dc412a..fa1acc3770 100644 --- a/src/libqof/qof/kvp_frame.h +++ b/src/libqof/qof/kvp_frame.h @@ -315,10 +315,10 @@ KvpValue * kvp_frame_get_value(const KvpFrame *frame, const gchar *path); * exist. */ /*@ dependent @*/ -KvpFrame * kvp_frame_get_frame(const KvpFrame *frame, const gchar *path); +KvpFrame* kvp_frame_get_frame(const KvpFrame *frame, const gchar *path); -KvpFrame * kvp_frame_get_frame_slash (KvpFrame *frame, - const gchar *path); +KvpFrame* kvp_frame_get_frame_slash (KvpFrame *frame, + const gchar *path); /** @} */ /** @name KvpFrame KvpValue low-level storing routines. diff --git a/src/libqof/qof/kvp_frame.hpp b/src/libqof/qof/kvp_frame.hpp index 3f5e3dcdfc..8d5b43de05 100644 --- a/src/libqof/qof/kvp_frame.hpp +++ b/src/libqof/qof/kvp_frame.hpp @@ -41,9 +41,11 @@ class cstring_comparer } }; +using Path = std::vector; + struct KvpFrameImpl { - typedef std::map map_type; + typedef std::map map_type; public: KvpFrameImpl() noexcept {}; @@ -54,22 +56,67 @@ struct KvpFrameImpl KvpFrameImpl(const KvpFrameImpl &) noexcept; /** - * Replaces the KvpValueImpl at the specified spot, and returns - * the old KvpValueImpl which used to occupy the spot. - * - * If no KvpValueImpl was at the spot, nullptr is returned. + * 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. + * @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. + */ + KvpValue* set(const char * key, KvpValue* newvalue) noexcept; + /** + * 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. + * @throw invalid_argument if the path doesn't exist. + * @param path: The path of subframes 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 path, KvpValue* newvalue) noexcept; + /** + * 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. + * @param path: The path of subframes 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(Path 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. */ - KvpValueImpl * replace_nc(const char * key, KvpValueImpl * newvalue) noexcept; - std::string to_string() const noexcept; - + /** + * Report the keys in the immediate frame. Be sensible about using this, it + * isn't a very efficient way to iterate. + * @return std::vector of keys as std::strings. + */ std::vector get_keys() const noexcept; - KvpValueImpl * get_slot(const char * key) const noexcept; + /** Get the value for the key or nullptr if it doesn't exist. + * @param key: The key. + * @return The value at the key or nullptr. + */ + KvpValue* get_slot(const char * key) const noexcept; + /** Get the value for the tail of the path or nullptr if it doesn't exist. + * @param path: Path of keys leading to the desired value. + * @return The value at the key or nullptr. + */ + KvpValue* get_slot(Path keys) const noexcept; + /** Convenience wrapper for std::for_each, which should be preferred. + */ + void for_each_slot(void (*proc)(const char *key, KvpValue *value, + void * data), + void *data) const noexcept; - void for_each_slot(void (*proc)(const char *key, KvpValue *value, void * data), void *data) const noexcept; - - friend int compare(const KvpFrameImpl &, const KvpFrameImpl &) noexcept; + /** Test for emptiness + * @return true if the frame contains nothing. + */ + bool empty() const noexcept { return m_valuemap.empty(); } + friend int compare(const KvpFrameImpl&, const KvpFrameImpl&) noexcept; private: map_type m_valuemap; diff --git a/src/libqof/qof/test/test-kvp-frame.cpp b/src/libqof/qof/test/test-kvp-frame.cpp index 432c811f8c..0fa9e45f79 100644 --- a/src/libqof/qof/test/test-kvp-frame.cpp +++ b/src/libqof/qof/test/test-kvp-frame.cpp @@ -27,6 +27,24 @@ #include #include +class KvpFrameTest : public ::testing::Test +{ +public: + KvpFrameTest() : + t_int_val{new KvpValue {INT64_C(15)}}, + t_str_val{new KvpValue{"a value"}} { + auto f1 = new KvpFrame; + t_root.set("top", new KvpValue{f1}); + f1->set("first", t_int_val); + f1->set("second", new KvpValue{new KvpFrame}); + f1->set("third", t_str_val); + } +protected: + KvpFrameImpl t_root; + KvpValue *t_int_val; + KvpValue *t_str_val; +}; + template void assert_contains (std::vector vec, B const & b) { @@ -34,57 +52,98 @@ assert_contains (std::vector vec, B const & b) EXPECT_NE (val, vec.end ()); } -TEST (KvpFrameTest, Replace) +TEST_F (KvpFrameTest, SetLocal) { auto f1 = new KvpFrameImpl; auto v1 = new KvpValueImpl {15.0}; auto v2 = new KvpValueImpl { (int64_t)52}; std::string k1 {"first key"}; - EXPECT_EQ (nullptr, f1->replace_nc (k1.c_str (), v1)); - EXPECT_EQ (v1, f1->replace_nc (k1.c_str (), v2)); + EXPECT_EQ (nullptr, f1->set (k1.c_str (), v1)); + EXPECT_EQ (v1, f1->set (k1.c_str (), v2)); delete f1; //this should also delete v2. delete v1; } -TEST (KvpFrameTest, GetKeys) +TEST_F (KvpFrameTest, SetPath) { - auto k1 = "first key"; - auto k2 = "second key"; - auto k3 = "first key/third key"; - auto f1 = new KvpFrameImpl; - auto v1 = new KvpValueImpl {15.2}; - auto v2 = new KvpValueImpl { (int64_t)12}; - auto v3 = new KvpValueImpl {strdup ("Never again")}; + Path path1 {"top", "second", "twenty-first"}; + Path path2 {"top", "third", "thirty-first"}; + auto v1 = new KvpValueImpl {15.0}; + auto v2 = new KvpValueImpl { (int64_t)52}; - f1->replace_nc (k1, v1); - f1->replace_nc (k2, v2); - f1->replace_nc (k3, v3); + EXPECT_EQ (nullptr, t_root.set(path1, v1)); + EXPECT_EQ (v1, t_root.set(path1, v2)); + EXPECT_EQ (nullptr, t_root.set(path2, v1)); + EXPECT_EQ (v2, t_root.get_slot(path1)); + EXPECT_EQ (nullptr, t_root.get_slot(path2)); + delete v1; +} - auto keys = f1->get_keys (); - EXPECT_EQ (keys.size (), 3); +TEST_F (KvpFrameTest, SetPathWithCreate) +{ + Path path1 {"top", "second", "twenty-first"}; + Path path2 {"top", "third", "thirty-first"}; + auto v1 = new KvpValueImpl {15.0}; + auto v2 = new KvpValueImpl { (int64_t)52}; + + EXPECT_EQ (nullptr, t_root.set_path(path1, v1)); + EXPECT_EQ (v1, t_root.set_path(path1, v2)); + EXPECT_EQ (nullptr, t_root.set_path(path2, v1)); + EXPECT_EQ (v2, t_root.get_slot(path1)); + EXPECT_EQ (v1, t_root.get_slot(path2)); +} + +TEST_F (KvpFrameTest, GetKeys) +{ + auto k1 = "top"; + auto k2 = "first"; + auto k3 = "second"; + + auto keys = t_root.get_keys (); + EXPECT_EQ (keys.size (), 1); assert_contains (keys, k1); + auto frameval = t_root.get_slot(k1); + ASSERT_EQ(frameval->get_type(), KVP_TYPE_FRAME); + keys = frameval->get()->get_keys(); assert_contains (keys, k2); assert_contains (keys, k3); - - delete f1;//This should delete our KvpValueImpls, and our string above, too. } -TEST (KvpFrameTest, GetSlot) +TEST_F (KvpFrameTest, GetLocalSlot) { - auto f1 = new KvpFrameImpl; - auto v1 = new KvpValueImpl {2.2}; - auto v2 = new KvpValueImpl { (int64_t)4}; - auto k1 = "first key"; - auto k2 = "first key/second key"; + auto k1 = "first"; + auto k2 = "third"; + auto k3 = "doesn't exist"; - f1->replace_nc (k1, v1); - EXPECT_EQ (v1, f1->get_slot(k1)); - f1->replace_nc (k2, v2); - EXPECT_EQ (v2, f1->get_slot(k2)); - EXPECT_EQ (v1, f1->get_slot(k1)); - - delete f1;//which will delete both kvpvalues, too. + auto frameval = t_root.get_slot("top"); + ASSERT_EQ(frameval->get_type(), KVP_TYPE_FRAME); + auto f1 = frameval->get(); + EXPECT_EQ (t_int_val, f1->get_slot(k1)); + EXPECT_EQ (t_str_val, f1->get_slot(k2)); + EXPECT_EQ (nullptr, f1->get_slot(k3)); +} + +TEST_F (KvpFrameTest, GetSlotPath) +{ + Path path1 {"top", "second", "twenty-first"}; + Path path2 {"top", "third", "thirty-first"}; + auto v1 = new KvpValueImpl {15.0}; + auto v2 = new KvpValueImpl { (int64_t)52}; + + EXPECT_EQ (nullptr, t_root.set(path1, v1)); + EXPECT_EQ (nullptr, t_root.set(path2, v2)); + EXPECT_EQ (v1, t_root.get_slot(path1)); + EXPECT_EQ (nullptr, t_root.get_slot(path2)); + delete v2; +} + +TEST_F (KvpFrameTest, Empty) +{ + KvpFrameImpl f1, f2; + f2.set("value", new KvpValue {2.2}); + EXPECT_TRUE(f1.empty()); + EXPECT_FALSE(f2.empty()); }