From 19f08da56b4b556c133aa05ebfa2333641a12a7b Mon Sep 17 00:00:00 2001
From: John Ralls <jralls@ceridwen.us>
Date: Tue, 18 Nov 2014 17:00:04 -0800
Subject: [PATCH] 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           | 46 ++++++++++++++++++++------
 src/libqof/qof/kvp-value.hpp           | 18 +++++-----
 src/libqof/qof/test/test-kvp-value.cpp |  9 +++++
 3 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/src/libqof/qof/kvp-value.cpp b/src/libqof/qof/kvp-value.cpp
index f92e71130d..3a8519daa6 100644
--- a/src/libqof/qof/kvp-value.cpp
+++ b/src/libqof/qof/kvp-value.cpp
@@ -30,16 +30,27 @@
 
 KvpValueImpl::KvpValueImpl(KvpValueImpl const & 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;
+    duplicate(other);
+}
+
+KvpValueImpl&
+KvpValueImpl::operator=(KvpValueImpl const & other) noexcept
+{
+    duplicate(other);
+    return *this;
+}
+
+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;
+}
diff --git a/src/libqof/qof/kvp-value.hpp b/src/libqof/qof/kvp-value.hpp
index e6f064c2b8..bfdfc33895 100644
--- a/src/libqof/qof/kvp-value.hpp
+++ b/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,
diff --git a/src/libqof/qof/test/test-kvp-value.cpp b/src/libqof/qof/test/test-kvp-value.cpp
index 66235cde38..537a6a0382 100644
--- a/src/libqof/qof/test/test-kvp-value.cpp
+++ b/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);
+}