diff --git a/Code/GraphMol/FileParsers/MultithreadedSDMolSupplier.cpp b/Code/GraphMol/FileParsers/MultithreadedSDMolSupplier.cpp index 5de3206dd..c24d5330f 100644 --- a/Code/GraphMol/FileParsers/MultithreadedSDMolSupplier.cpp +++ b/Code/GraphMol/FileParsers/MultithreadedSDMolSupplier.cpp @@ -183,6 +183,9 @@ void MultithreadedSDMolSupplier::readMolProps(ROMol *mol, while (stmp.length() != 0) { std::getline(inStream, tempStr); if (inStream.eof()) { + if (mol) { + delete mol; + } throw FileParseException("End of data field name not found"); } } @@ -228,6 +231,9 @@ void MultithreadedSDMolSupplier::readMolProps(ROMol *mol, // and issue a warning // FIX: should we be deleting the molecule (which is probably fine) // because we couldn't read the data ??? + if (mol) { + delete mol; + } throw FileParseException("Problems encountered parsing data fields"); } else { if (!warningIssued) { diff --git a/Code/GraphMol/FileParsers/testMultithreadedMolSupplier.cpp b/Code/GraphMol/FileParsers/testMultithreadedMolSupplier.cpp index 1f5744b59..e2563179f 100644 --- a/Code/GraphMol/FileParsers/testMultithreadedMolSupplier.cpp +++ b/Code/GraphMol/FileParsers/testMultithreadedMolSupplier.cpp @@ -7,6 +7,10 @@ // which is included in the file license.txt, found at the root // of the RDKit source tree. // + +#include +#include + #include #include #include @@ -15,7 +19,6 @@ #include #include #include -#include #include "MultithreadedSDMolSupplier.h" #include "MultithreadedSmilesMolSupplier.h" @@ -34,7 +37,7 @@ struct PrintThread : public std::stringstream { } }; -void testSmiConcurrent(std::istream* strm, bool takeOwnership, +void testSmiConcurrent(std::istream *strm, bool takeOwnership, std::string delimiter, int smilesColumn, int nameColumn, bool titleLine, bool sanitize, unsigned int numWriterThreads, size_t sizeInputQueue, @@ -51,7 +54,7 @@ void testSmiConcurrent(std::istream* strm, bool takeOwnership, TEST_ASSERT(!bitVector.any()); while (!sup.atEnd()) { - ROMol* mol = sup.next(); + ROMol *mol = sup.next(); if (mol) { unsigned int id = sup.getLastRecordId(); bitVector[id - 1] = 1; @@ -75,7 +78,7 @@ void testSmiConcurrent(std::string path, std::string delimiter, unsigned int expectedResult, bool extras = false) { std::string rdbase = getenv("RDBASE"); std::string fname = rdbase + path; - std::istream* strm = new std::ifstream(fname.c_str()); + std::istream *strm = new std::ifstream(fname.c_str()); testSmiConcurrent(strm, true, delimiter, smilesColumn, nameColumn, titleLine, sanitize, numWriterThreads, sizeInputQueue, sizeOutputQueue, expectedResult, extras); @@ -88,7 +91,7 @@ void testSmiOld(std::string path, std::string delimiter, int smilesColumn, SmilesMolSupplier sup(path, delimiter, smilesColumn, nameColumn, titleLine, sanitize); while (!sup.atEnd()) { - ROMol* mol = sup.next(); + ROMol *mol = sup.next(); if (mol) { if (extras) { std::unique_ptr fp( @@ -110,7 +113,7 @@ void testSmiProperties() { SmilesMolSupplier sup(fname, ",", 1, 0, true); MultithreadedSmilesMolSupplier multiSup(fname, ",", 1, 0, true); while (!multiSup.atEnd()) { - ROMol* mol = multiSup.next(); + std::unique_ptr mol{multiSup.next()}; if (mol != nullptr) { mol->getProp(common_properties::_Name, tempStr); nameVector.push_back(tempStr); @@ -120,7 +123,7 @@ void testSmiProperties() { } while (!sup.atEnd()) { - ROMol* mol = sup.next(); + std::unique_ptr mol{sup.next()}; if (mol != nullptr) { mol->getProp(common_properties::_Name, tempStr); TEST_ASSERT(std::find(nameVector.begin(), nameVector.end(), tempStr) != @@ -153,7 +156,7 @@ void testSmiCorrectness() { #ifdef RDK_USE_BOOST_IOSTREAMS path = rdbase + "/Regress/Data/znp.50k.smi.gz"; - std::istream* strm = new gzstream(path); + std::istream *strm = new gzstream(path); expectedResult = 50000; testSmiConcurrent(strm, true, " \t", 0, 1, false, false, 3, 1000, 100, expectedResult); @@ -166,7 +169,7 @@ void testSmiCorrectness() { testSmiProperties(); } -void testSDConcurrent(std::istream* strm, bool takeOwnership, bool sanitize, +void testSDConcurrent(std::istream *strm, bool takeOwnership, bool sanitize, bool removeHs, bool strictParsing, unsigned int numWriterThreads, size_t sizeInputQueue, size_t sizeOutputQueue, unsigned int expectedResult, @@ -181,7 +184,7 @@ void testSDConcurrent(std::istream* strm, bool takeOwnership, bool sanitize, // initially no bit is set in the bitVector, sanity check TEST_ASSERT(!bitVector.any()); while (!sup.atEnd()) { - ROMol* mol = sup.next(); + ROMol *mol = sup.next(); if (mol) { unsigned int id = sup.getLastRecordId(); bitVector[id - 1] = 1; @@ -204,7 +207,7 @@ void testSDConcurrent(std::string path, bool sanitize, bool removeHs, unsigned int expectedResult, bool extras = false) { std::string rdbase = getenv("RDBASE"); std::string fname = rdbase + path; - std::istream* strm = new std::ifstream(fname.c_str()); + std::istream *strm = new std::ifstream(fname.c_str()); testSDConcurrent(strm, true, sanitize, removeHs, strictParsing, numWriterThreads, sizeInputQueue, sizeOutputQueue, expectedResult, extras); @@ -220,7 +223,7 @@ void testSDProperties() { MultithreadedSDMolSupplier multiSup(fname, false); while (!multiSup.atEnd()) { - ROMol* mol = multiSup.next(); + std::unique_ptr mol{multiSup.next()}; if (mol != nullptr) { TEST_ASSERT(mol->hasProp(common_properties::_Name)); mol->getProp(common_properties::_Name, tempStr); @@ -232,7 +235,7 @@ void testSDProperties() { } while (!sup.atEnd()) { - ROMol* mol = sup.next(); + std::unique_ptr mol{sup.next()}; if (mol != nullptr) { mol->getProp(common_properties::_Name, tempStr); TEST_ASSERT(std::find(nameVector.begin(), nameVector.end(), tempStr) != @@ -247,7 +250,7 @@ void testSDOld(std::string path, bool sanitize, bool removeHs, unsigned int numMols = 0; SDMolSupplier sup(path, sanitize, removeHs, strictParsing); while (!sup.atEnd()) { - ROMol* mol = sup.next(); + ROMol *mol = sup.next(); if (mol) { ++numMols; if (extras) { @@ -290,7 +293,7 @@ void testSDCorrectness() { #ifdef RDK_USE_BOOST_IOSTREAMS path = rdbase + "/Regress/Data/mols.1000.sdf.gz"; - std::istream* strm = new gzstream(path); + std::istream *strm = new gzstream(path); expectedResult = 1000; testSDConcurrent(strm, true, false, true, true, 2, 5, 5, expectedResult); @@ -337,7 +340,7 @@ void testPerformance() { << " (milliseconds) \n"; for (unsigned int i = maxThreadCount; i >= 1; --i) { - std::istream* strm = new gzstream(rdbase + gzpath); + std::istream *strm = new gzstream(rdbase + gzpath); start = high_resolution_clock::now(); bool takeOwnership = true; testSmiConcurrent(strm, takeOwnership, delim, smilesColumn, nameColumn, @@ -374,7 +377,7 @@ void testPerformance() { << " (milliseconds) \n"; for (unsigned int i = maxThreadCount; i >= 1; --i) { - std::istream* strm = new gzstream(rdbase + gzpath); + std::istream *strm = new gzstream(rdbase + gzpath); bool takeOwnership = true; start = high_resolution_clock::now(); testSDConcurrent(strm, takeOwnership, sanitize, removeHs, strictParsing, diff --git a/Code/GraphMol/Resonance.cpp b/Code/GraphMol/Resonance.cpp index 8b1d7e705..c4cd21aff 100644 --- a/Code/GraphMol/Resonance.cpp +++ b/Code/GraphMol/Resonance.cpp @@ -132,7 +132,7 @@ class ConjElectrons { bool checkChargesAndBondOrders(); void computeMetrics(); bool checkMetrics(CEStats &ceStats, bool &ok) const; - void purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount, CEStats &ceStats) const; + bool purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount, CEStats &ceStats) const; void updateDegCount(CEDegCount &ceDegCount); std::size_t computeFP(unsigned int flags); inline bool haveFP(CESet &ceSet, unsigned int flags); @@ -296,11 +296,7 @@ void fixExplicitImplicitHs(ROMol &mol) { // object constructor AtomElectrons::AtomElectrons(ConjElectrons *parent, const Atom *a) - : d_nb(0), - d_fc(0), - d_flags(0), - d_atom(a), - d_parent(parent) { + : d_nb(0), d_fc(0), d_flags(0), d_atom(a), d_parent(parent) { PRECONDITION(d_atom, "d_atom cannot be NULL"); d_tv = static_cast(a->getTotalDegree()); const ROMol &mol = d_atom->getOwningMol(); @@ -833,23 +829,28 @@ bool ConjElectrons::checkMetrics(CEStats &ceStats, bool &changed) const { return ok; } -void ConjElectrons::purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount, +bool ConjElectrons::purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount, CEStats &ceStats) const { + bool ok = true; bool changed = true; while (changed) { - for (CEMap::iterator it = ceMap.begin(); it != ceMap.end();) { + for (auto it = ceMap.begin(); it != ceMap.end();) { if (!it->second->checkMetrics(ceStats, changed)) { - CEMap::iterator toBeDeleted = it; - ++it; - auto it2 = ceDegCount.find(toBeDeleted->second->hash()); + auto it2 = ceDegCount.find(it->second->hash()); if (it2 != ceDegCount.end()) { --it2->second; if (!it2->second) { ceDegCount.erase(it2); } } - delete toBeDeleted->second; - ceMap.erase(toBeDeleted); + if (it->second == this) { + // postpone self deletion + ok = false; + } else { + delete it->second; + } + it = ceMap.erase(it); + changed = true; } else { ++it; } @@ -858,6 +859,7 @@ void ConjElectrons::purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount, } } } + return ok; } // assign formal charges and, if they are acceptable, store @@ -877,7 +879,7 @@ bool ConjElectrons::assignFormalChargesAndStore(CEMap &ceMap, ok = storeFP(ceMap, fpFlags); } if (changed) { - purgeMaps(ceMap, ceDegCount, ceStats); + ok = purgeMaps(ceMap, ceDegCount, ceStats) && ok; } if (ok) { updateDegCount(ceDegCount); @@ -888,7 +890,6 @@ bool ConjElectrons::assignFormalChargesAndStore(CEMap &ceMap, // enumerate all possible permutations of non-bonded electrons void ConjElectrons::enumerateNonBonded(CEMap &ceMap, CEDegCount &ceDegCount, CEStats &ceStats) { - ConjElectrons *ce = this; // the way we compute FPs for a resonance structure depends // on whether we want to enumerate all Kekule structures // or not; in the first case, we also include bond orders @@ -926,12 +927,10 @@ void ConjElectrons::enumerateNonBonded(CEMap &ceMap, CEDegCount &ceDegCount, ResonanceUtils::getNumCombStartV(numCand, aiVec.size(), numComb, v); // if there are multiple permutations, make a copy of the original // ConjElectrons object, since the latter will be modified - ConjElectrons *ceCopy = ((numComb > 1) ? new ConjElectrons(*ce) : nullptr); + ConjElectrons *ceCopy = new ConjElectrons(*this); // enumerate all permutations for (unsigned int c = 0; c < numComb; ++c) { - if (c) { - ce = new ConjElectrons(*ceCopy); - } + ConjElectrons *ce = new ConjElectrons(*ceCopy); unsigned int vc = v; for (unsigned int i : aiVec) { AtomElectrons *ae = ce->getAtomElectronsWithIdx(i); @@ -951,11 +950,13 @@ void ConjElectrons::enumerateNonBonded(CEMap &ceMap, CEDegCount &ceDegCount, fpFlags)) { delete ce; } + // get the next binary code ResonanceUtils::updateV(v); } delete ceCopy; } else if (nbTotal == currElectrons()) { + ConjElectrons *ce = new ConjElectrons(*this); // if the electrons required to satisfy all octets // are as many as those currently available, assignment // is univocal @@ -964,12 +965,10 @@ void ConjElectrons::enumerateNonBonded(CEMap &ceMap, CEDegCount &ceDegCount, if (!ce->assignFormalChargesAndStore(ceMap, ceDegCount, ceStats, fpFlags)) { delete ce; } - } else { - // if the electrons required to satisfy all octets are less - // than those currently available, we must have failed the bond - // assignment, so the candidate must be deleted - delete ce; } + // if the electrons required to satisfy all octets are less + // than those currently available, we must have failed the bond + // assignment } void ConjElectrons::computeMetrics() { @@ -1694,6 +1693,7 @@ void ResonanceMolSupplier::buildCEMap(CEMap &ceMap, unsigned int conjGrpIdx) { // enumerate possible non-bonded electron // arrangements, and store them in ceMap ce->enumerateNonBonded(ceMap, ceDegCount, ceStats); + delete ce; // quit the loop early if the number of non-degenerate // structures already exceeds d_maxStructs if (d_callback.get()) { @@ -1711,6 +1711,10 @@ void ResonanceMolSupplier::buildCEMap(CEMap &ceMap, unsigned int conjGrpIdx) { } } } + while (!ceStack.empty()) { + delete ceStack.top(); + ceStack.pop(); + } } // getter function which returns the bondConjGrpIdx for a given diff --git a/Code/GraphMol/catch_graphmol.cpp b/Code/GraphMol/catch_graphmol.cpp index 3d9687cbb..dd4200505 100644 --- a/Code/GraphMol/catch_graphmol.cpp +++ b/Code/GraphMol/catch_graphmol.cpp @@ -1380,7 +1380,7 @@ TEST_CASE("Github #3470: Hydrogen is incorrectly identified as an early atom", "[bug][chemistry]") { SECTION("Basics") { RWMol m; - m.addAtom(new Atom(1)); + m.addAtom(new Atom(1), true, true); m.getAtomWithIdx(0)->setFormalCharge(-1); m.updatePropertyCache(); CHECK(m.getAtomWithIdx(0)->getNumImplicitHs() == 0); diff --git a/Contrib/ConformerParser/test.cpp b/Contrib/ConformerParser/test.cpp index 8653fc845..516aa068a 100644 --- a/Contrib/ConformerParser/test.cpp +++ b/Contrib/ConformerParser/test.cpp @@ -33,6 +33,7 @@ #include #include +#include #include #include @@ -57,7 +58,7 @@ void test1() { BOOST_LOG(rdErrorLog) << "-------------------------------------" << std::endl; BOOST_LOG(rdErrorLog) << " Test ConformerParser." << std::endl; - ROMol *mol = SmilesToMol("CCC"); + std::unique_ptr mol{SmilesToMol("CCC")}; std::vector> coords; std::string rdbase = getenv("RDBASE"); std::string fName = rdbase + "/Code/GraphMol/test_data/water_coords_bad.trx"; diff --git a/External/CoordGen/CoordGen.h b/External/CoordGen/CoordGen.h index 51e0f0176..e6a02c77c 100644 --- a/External/CoordGen/CoordGen.h +++ b/External/CoordGen/CoordGen.h @@ -215,6 +215,10 @@ unsigned int addCoords(T& mol, const CoordGenParams* params = nullptr) { } else { CoordgenFragmenter::splitIntoFragments(min_mol); minimizer.m_minimizer.minimizeMolecule(min_mol); + + // Hook the fragments into the minimizer so that they + // get cleaned up when the minimizer is terminated + minimizer._fragments = min_mol->getFragments(); } auto conf = new Conformer(mol.getNumAtoms()); for (size_t i = 0; i < mol.getNumAtoms(); ++i) { diff --git a/External/YAeHMOP/CMakeLists.txt b/External/YAeHMOP/CMakeLists.txt index 65d68e566..95e19831d 100644 --- a/External/YAeHMOP/CMakeLists.txt +++ b/External/YAeHMOP/CMakeLists.txt @@ -24,7 +24,7 @@ ExternalProject_Add(yaehmop_project PREFIX ${CMAKE_CURRENT_SOURCE_DIR} SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/yaehmop" SOURCE_SUBDIR "tightbind" - CMAKE_ARGS -DUSE_BLAS_LAPACK=OFF -DCMAKE_INSTALL_PREFIX=${PROJECT_BINARY_DIR} -DCMAKE_C_FLAGS=${CMAKE_C_FLAGS} + CMAKE_ARGS -DUSE_BLAS_LAPACK=OFF -DCMAKE_INSTALL_PREFIX=${PROJECT_BINARY_DIR} -DCMAKE_C_FLAGS=${CMAKE_C_FLAGS} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} TEST_COMMAND "") include_directories(${PROJECT_BINARY_DIR}/include)