Fixes rdkit#940 (rdkit#941)

* Fixes rdkit#940 * ensure RWMol ctors also initialize properly * cleanup when replacing non-POD with POD
giallu · Jun 14, 2016 · 455c188 · 455c188
1 parent 1cde16b
commit 455c188
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 3 deletions.
diff --git a/Code/GraphMol/RWMol.h b/Code/GraphMol/RWMol.h
@@ -29,7 +29,7 @@ namespace RDKit {
  */
 class RWMol : public ROMol {
  public:
-  RWMol() { d_partialBonds.clear(); }
+  RWMol() : ROMol() { d_partialBonds.clear(); }
 
   //! copy constructor with a twist
   /*!
@@ -41,9 +41,9 @@ class RWMol : public ROMol {
     \param confId if this is >=0, the resulting ROMol will contain only
          the specified conformer from \c other.
   */
-  RWMol(const ROMol &other, bool quickCopy = false, int confId = -1) {
+  RWMol(const ROMol &other, bool quickCopy = false, int confId = -1)
+      : ROMol(other, quickCopy, confId) {
     d_partialBonds.clear();
-    initFromOther(other, quickCopy, confId);
   };
   RWMol &operator=(const RWMol &);
 

diff --git a/Code/RDGeneral/Dict.h b/Code/RDGeneral/Dict.h
@@ -178,6 +178,7 @@ class Dict {
     _hasNonPodData = true;
     for(size_t i=0; i< _data.size(); ++i) {
       if (_data[i].key == what) {
+        RDValue::cleanup_rdvalue(_data[i].val);
         _data[i].val = val;
         return;
       }
@@ -190,6 +191,7 @@ class Dict {
     // don't change the hasNonPodData status
     for(size_t i=0; i< _data.size(); ++i) {
       if (_data[i].key == what) {
+        RDValue::cleanup_rdvalue(_data[i].val);
         _data[i].val = val;
         return;
       }
@@ -237,6 +239,9 @@ class Dict {
   void clearVal(const std::string &what) {
     for(DataType::iterator it = _data.begin(); it < _data.end() ; ++it) {
       if (it->key == what) {
+        if (_hasNonPodData) {
+          RDValue::cleanup_rdvalue(it->val);
+        }
         _data.erase(it);
         return;
       }

diff --git a/Code/RDGeneral/testDict.cpp b/Code/RDGeneral/testDict.cpp
@@ -29,6 +29,31 @@ struct Foo {
   ~Foo() { std::cerr << "deleted!" << std::endl; }
 };
 
+void testGithub940() {
+  BOOST_LOG(rdErrorLog)
+      << "Testing Github940: property dictionaries leaking memory" << std::endl;
+
+  // a couple small tests to check for memory leaks. Only useful with valgrind
+  {  // tests computed props
+    STR_VECT computed;
+    Dict *d = new Dict();
+    d->setVal(detail::computedPropName, computed);
+    computed.push_back("foo");
+    d->setVal(detail::computedPropName, computed);
+    delete d;
+  }
+  {  // tests computed props
+    STR_VECT computed;
+    Dict *d = new Dict();
+    d->setVal(detail::computedPropName, computed);
+    computed.push_back("foo");
+    d->setVal(detail::computedPropName, computed);
+    d->clearVal(detail::computedPropName);
+    delete d;
+  }
+  BOOST_LOG(rdErrorLog) << "\tdone" << std::endl;
+}
+
 void testRDAny() {
   std::cerr << "Testing RDValue" << std::endl;
   {
@@ -546,6 +571,8 @@ void testConstReturns() {
 
 int main() {
   RDLog::InitLogs();
+  testGithub940();
+  testRDAny();
     testRDAny();
 
 #if 1