gnucash master: Multiple changes pushed
John Ralls
jralls at code.gnucash.org
Tue Nov 18 20:01:45 EST 2014
Updated via https://github.com/Gnucash/gnucash/commit/19f08da5 (commit)
via https://github.com/Gnucash/gnucash/commit/ba59350f (commit)
from https://github.com/Gnucash/gnucash/commit/578dfd81 (commit)
commit 19f08da56b4b556c133aa05ebfa2333641a12a7b
Author: John Ralls <jralls at ceridwen.us>
Date: Tue Nov 18 17:00:04 2014 -0800
Implement copy and move operator= and move constructor for KvpValueImpl
Fixes double-delete crash of the embedded ptr when copy-initializing.
diff --git a/src/libqof/qof/kvp-value.cpp b/src/libqof/qof/kvp-value.cpp
index f92e711..3a8519d 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 e6f064c..bfdfc33 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 66235cd..537a6a0 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);
+}
commit ba59350f69afbef6b301e9780f5105931e470dcb
Author: lmat <dartme18 at gmail.com>
Date: Fri Oct 3 22:05:37 2014 -0400
Adding KvpValueImpl test suite
diff --git a/src/libqof/qof/kvp-value.hpp b/src/libqof/qof/kvp-value.hpp
index bf1b616..e6f064c 100644
--- a/src/libqof/qof/kvp-value.hpp
+++ b/src/libqof/qof/kvp-value.hpp
@@ -35,6 +35,16 @@ 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:
diff --git a/src/libqof/qof/test/Makefile.am b/src/libqof/qof/test/Makefile.am
index fc77ecd..2641069 100644
--- a/src/libqof/qof/test/Makefile.am
+++ b/src/libqof/qof/test/Makefile.am
@@ -48,3 +48,23 @@ test_qof_CPPFLAGS = \
-DTESTPROG=test_qof \
-I$(top_srcdir)/lib/libc \
${GLIB_CFLAGS}
+
+if WITH_GOOGLE_TEST
+test_kvp_value_SOURCES = \
+ $(top_srcdir)/$(MODULEPATH)/kvp-value.cpp \
+ $(GTEST_ROOT)/src/gtest_main.cc \
+ test-kvp-value.cpp
+test_kvp_value_LDADD = \
+ $(top_builddir)/$(MODULEPATH)/libgnc-qof.la \
+ $(top_builddir)/src/test-core/libgtest.a \
+ $(GLIB_LIBS) \
+ $(BOOST_LDFLAGS)
+test_kvp_value_CPPFLAGS = \
+ -I$(GTEST_HEADERS) \
+ -I$(top_srcdir)/$(MODULEPATH) \
+ $(BOOST_CPPFLAGS) \
+ $(GLIB_CFLAGS)
+
+check_PROGRAMS += test_kvp_value
+
+endif
diff --git a/src/libqof/qof/test/test-kvp-value.cpp b/src/libqof/qof/test/test-kvp-value.cpp
new file mode 100644
index 0000000..66235cd
--- /dev/null
+++ b/src/libqof/qof/test/test-kvp-value.cpp
@@ -0,0 +1,90 @@
+/********************************************************************
+ * test-kvp-value.cpp: A Google Test suite for kvp-value impl. *
+ * Copyright 2014 Aaron Laws <dartme18 at gmail.com> *
+ * *
+ * This program is free software; you can redistribute it and/or *
+ * modify it under the terms of the GNU General Public License as *
+ * published by the Free Software Foundation; either version 2 of *
+ * the License, or (at your option) any later version. *
+ * *
+ * This program is distributed in the hope that it will be useful, *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
+ * GNU General Public License for more details. *
+ * *
+ * You should have received a copy of the GNU General Public License*
+ * along with this program; if not, you can retrieve it from *
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html *
+ * or contact: *
+ * *
+ * Free Software Foundation Voice: +1-617-542-5942 *
+ * 51 Franklin Street, Fifth Floor Fax: +1-617-542-2652 *
+ * Boston, MA 02110-1301, USA gnu at gnu.org *
+ ********************************************************************/
+
+#include "../kvp-value.hpp"
+#include "../guid.h"
+#include "../kvp_frame.hpp"
+#include "../gnc-date.h"
+#include <memory>
+#include <gtest/gtest.h>
+
+TEST (KvpValueTest, Replace_Frame)
+{
+ auto f1 = new KvpFrameImpl;
+ std::unique_ptr<KvpValueImpl> v1 {new KvpValueImpl {f1}};
+ auto f2 = new KvpFrameImpl;
+ EXPECT_EQ (f1, v1->replace_frame_nc (f2));
+ v1->set (5.2);
+ /*f1 and f2 should be deleted now*/
+ f1 = new KvpFrameImpl;
+ EXPECT_EQ (nullptr, v1->replace_frame_nc (f1));
+ delete f1;
+}
+
+TEST (KvpValueTest, Equality)
+{
+ std::unique_ptr<KvpValueImpl> v1 {new KvpValueImpl { (int64_t)1}};
+ std::unique_ptr<KvpValueImpl> v2 {new KvpValueImpl { (int64_t)2}};
+ EXPECT_LT (compare (*v1, *v2), 0);
+ // This is deleted in the kvpvalue destructor!
+ auto guid = guid_new ();
+ v1 = std::unique_ptr<KvpValueImpl> {new KvpValueImpl {guid}};
+ //this guid is also deleted in kvp value destructor.
+ v2 = std::unique_ptr<KvpValueImpl> {new KvpValueImpl {guid_copy (guid)}};
+ EXPECT_EQ (compare (*v1, *v2), 0);
+
+ v1 = std::unique_ptr<KvpValueImpl> {new KvpValueImpl {timespec_now ()}};
+ v2 = std::unique_ptr<KvpValueImpl> {new KvpValueImpl {*v1}};
+ EXPECT_EQ (compare (*v1, *v2), 0);
+}
+
+TEST (KvpValueTest, Add)
+{
+ auto v3 = new KvpValueImpl {timespec_now ()};
+ auto v4 = new KvpValueImpl {*v3};
+ auto new_one = v3->add (v4);
+ EXPECT_NE (new_one, v3);
+ EXPECT_EQ (new_one->get_type (), KvpValueType::KVP_TYPE_GLIST);
+ EXPECT_NE (new_one->get<GList*> (), nullptr);
+ /* also deletes v3 and v4 because they're "in" new_one */
+ delete new_one;
+}
+
+TEST (KvpValueTest, Copy)
+{
+ std::unique_ptr<KvpValueImpl> v1 {new KvpValueImpl {5.2}};
+ std::unique_ptr<KvpValueImpl> v2 {new KvpValueImpl {*v1}};
+ EXPECT_EQ (compare (*v1, *v2), 0);
+
+ auto guid = guid_new ();
+ v1 = std::unique_ptr<KvpValueImpl> {new KvpValueImpl {guid}};
+ v2 = std::unique_ptr<KvpValueImpl> {new KvpValueImpl {*v1}};
+ EXPECT_EQ (compare (*v1, *v2), 0);
+
+ v1 = std::unique_ptr<KvpValueImpl> {new KvpValueImpl {guid}};
+ v2 = std::unique_ptr<KvpValueImpl> {new KvpValueImpl {*v1}};
+ /*This should delete the guid*/
+ v1->set(5.2);
+ EXPECT_NE (compare (*v1, *v2), 0);
+}
Summary of changes:
src/libqof/qof/kvp-value.cpp | 46 ++++++++++++----
src/libqof/qof/kvp-value.hpp | 8 +++
src/libqof/qof/test/Makefile.am | 20 +++++++
src/libqof/qof/test/test-kvp-value.cpp | 99 ++++++++++++++++++++++++++++++++++
4 files changed, 163 insertions(+), 10 deletions(-)
create mode 100644 src/libqof/qof/test/test-kvp-value.cpp
More information about the gnucash-changes
mailing list