Implement copy and move operator= and move constructor for KvpValueImpl

Fixes double-delete crash of the embedded ptr when copy-initializing.

src/libqof/qof/kvp-value.cpp

@@ -30,16 +30,27 @@
 
 KvpValueImpl::KvpValueImpl(KvpValueImpl const & other) noexcept
 {
-    if (other.datastore.type() == typeid(gchar *))
+    duplicate(other);
-        this->datastore = g_strdup(other.get<gchar *>());
+}
-    else if (other.datastore.type() == typeid(GncGUID*))
+
-        this->datastore = guid_copy(other.get<GncGUID *>());
+KvpValueImpl&
-    else if (other.datastore.type() == typeid(GList*))
+KvpValueImpl::operator=(KvpValueImpl const & other) noexcept
-        this->datastore = kvp_glist_copy(other.get<GList *>());
+{
-    else if (other.datastore.type() == typeid(KvpFrame*))
+    duplicate(other);
-        this->datastore = kvp_frame_copy(other.get<KvpFrame *>());
+    return *this;
-    else
+}
-        this->datastore = other.datastore;
+
+KvpValueImpl::KvpValueImpl(KvpValueImpl && b) noexcept
+{
+    datastore = b.datastore;
+    b.datastore = INT64_C(0);
+}
+
+KvpValueImpl&
+KvpValueImpl::operator=(KvpValueImpl && b) noexcept
+{
+    std::swap (datastore, b.datastore);
+    return *this;
 }
 
 KvpValueImpl *
@@ -322,3 +333,18 @@ KvpValueImpl::~KvpValueImpl() noexcept
     delete_visitor d;
     boost::apply_visitor(d, datastore);
 }
+
+void
+KvpValueImpl::duplicate(const KvpValueImpl& other) noexcept
+{
+    if (other.datastore.type() == typeid(gchar *))
+        this->datastore = g_strdup(other.get<gchar *>());
+    else if (other.datastore.type() == typeid(GncGUID*))
+        this->datastore = guid_copy(other.get<GncGUID *>());
+    else if (other.datastore.type() == typeid(GList*))
+        this->datastore = kvp_glist_copy(other.get<GList *>());
+    else if (other.datastore.type() == typeid(KvpFrame*))
+        this->datastore = kvp_frame_copy(other.get<KvpFrame *>());
+    else
+        this->datastore = other.datastore;
+}

src/libqof/qof/kvp-value.hpp

@@ -35,16 +35,6 @@ extern "C"
 #endif
 #include <boost/variant.hpp>
 
-/**
- * KvpValueImpl should generally not be used on the stack because its
- * destructor frees its contents, and it doesn't know anything
- * about move semantics. Cases such as
- *     KvpValueImpl v1;
- *     auto guid = guid_new ();
- *     v1 = KvpValueImpl {guid};
- * fail because the third line deletes the guid in KvpValueImpl's destructor.
- * Passing by value has similar problems.
- */
 struct KvpValueImpl
 {
     public:
@@ -52,6 +42,13 @@ struct KvpValueImpl
      * Performs a deep copy
      */
     KvpValueImpl(KvpValueImpl const &) noexcept;
+    KvpValueImpl& operator=(const KvpValueImpl&) noexcept;
+
+    /**
+     * Move. The old object's datastore is set to int646_t 0.
+     */
+    KvpValueImpl(KvpValueImpl && b) noexcept;
+    KvpValueImpl& operator=(KvpValueImpl && b) noexcept;
 
     template <typename T>
     KvpValueImpl(T) noexcept;
@@ -105,6 +102,7 @@ struct KvpValueImpl
     friend int compare(const KvpValueImpl &, const KvpValueImpl &) noexcept;
 
     private:
+    void duplicate(const KvpValueImpl&) noexcept;
     boost::variant<
         int64_t,
         double,

src/libqof/qof/test/test-kvp-value.cpp

@@ -88,3 +88,12 @@ TEST (KvpValueTest, Copy)
     v1->set(5.2);
     EXPECT_NE (compare (*v1, *v2), 0);
 }
+
+TEST (KvpValueTest, Stack)
+{
+    KvpValueImpl v1 {5.2};
+    auto guid = guid_new ();
+    v1 = KvpValueImpl {guid};
+
+    EXPECT_NE (nullptr, guid);
+}