From 4a57ef65d75c46efa3ea970bbc9d0f4c670a71ae Mon Sep 17 00:00:00 2001
From: Richard Roberts <richard.jw.roberts@gmail.com>
Date: Fri, 17 Aug 2012 03:45:35 +0000
Subject: [PATCH] Fixed memory leak when a Value contained heap-allocated data
 (Matrix, Vector, etc) - incorrect syntax when calling destructor did not do a
 virtual call so derived Value object was not cleaned up.

---
 gtsam/base/DerivedValue.h            |  4 +--
 gtsam/nonlinear/tests/testValues.cpp | 41 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/gtsam/base/DerivedValue.h b/gtsam/base/DerivedValue.h
index 52420ee93..2dc6f3d67 100644
--- a/gtsam/base/DerivedValue.h
+++ b/gtsam/base/DerivedValue.h
@@ -47,8 +47,8 @@ public:
    * Destroy and deallocate this object, only if it was originally allocated using clone_().
    */
   virtual void deallocate_() const {
-    this->Value::~Value();
-  	boost::singleton_pool<PoolTag, sizeof(DERIVED)>::free((void*)this);
+    this->~DerivedValue(); // Virtual destructor cleans up the derived object
+  	boost::singleton_pool<PoolTag, sizeof(DERIVED)>::free((void*)this); // Release memory from pool
   }
 
 	/**
diff --git a/gtsam/nonlinear/tests/testValues.cpp b/gtsam/nonlinear/tests/testValues.cpp
index 3818e99a2..5242fef1c 100644
--- a/gtsam/nonlinear/tests/testValues.cpp
+++ b/gtsam/nonlinear/tests/testValues.cpp
@@ -38,6 +38,27 @@ using symbol_shorthand::L;
 
 const Symbol key1('v',1), key2('v',2), key3('v',3), key4('v',4);
 
+
+class TestValueData {
+public:
+	static int ConstructorCount;
+	static int DestructorCount;
+	TestValueData(const TestValueData& other) { cout << "Copy constructor" << endl; ++ ConstructorCount; }
+	TestValueData() { cout << "Default constructor" << endl; ++ ConstructorCount; }
+	~TestValueData() { cout << "Destructor" << endl; ++ DestructorCount; }
+};
+int TestValueData::ConstructorCount = 0;
+int TestValueData::DestructorCount = 0;
+class TestValue : public DerivedValue<TestValue> {
+	TestValueData data_;
+public:
+	virtual void print(const std::string& str = "") const {}
+	bool equals(const TestValue& other, double tol = 1e-9) const { return true; }
+	virtual size_t dim() const { return 0; }
+	TestValue retract(const Vector&) const { return TestValue(); }
+	Vector localCoordinates(const TestValue&) const { return Vector(); }
+};
+
 /* ************************************************************************* */
 TEST( Values, equals1 )
 {
@@ -358,6 +379,26 @@ TEST(Values, Symbol_filter) {
   LONGS_EQUAL(2, i);
 }
 
+/* ************************************************************************* */
+TEST(Values, Destructors) {
+	// Check that Value destructors are called when Values container is deleted
+	{
+		Values values;
+		{
+			TestValue value1;
+			TestValue value2;
+			LONGS_EQUAL(2, TestValueData::ConstructorCount);
+			LONGS_EQUAL(0, TestValueData::DestructorCount);
+			values.insert(0, value1);
+			values.insert(1, value2);
+		}
+		LONGS_EQUAL(4, TestValueData::ConstructorCount);
+		LONGS_EQUAL(2, TestValueData::DestructorCount);
+	}
+	LONGS_EQUAL(4, TestValueData::ConstructorCount);
+	LONGS_EQUAL(4, TestValueData::DestructorCount);
+}
+
 /* ************************************************************************* */
 int main() { TestResult tr; return TestRegistry::runAllTests(tr); }
 /* ************************************************************************* */