From 157df3c2967ce2949b68ea3fed5d46b767484b17 Mon Sep 17 00:00:00 2001 From: Greg Landrum Date: Wed, 29 Sep 2021 09:31:36 +0200 Subject: [PATCH] fix a thread-safety bug in the UFF parameter acquisition (#4553) --- Code/ForceField/UFF/Params.cpp | 31 +++++++++++++------ Code/ForceField/UFF/Params.h | 13 ++++---- Code/ForceField/UFF/testUFFForceField.cpp | 12 +++---- .../ForceFieldHelpers/UFF/AtomTyper.cpp | 13 ++++---- .../ForceFieldHelpers/UFF/testUFFHelpers.cpp | 5 ++- ReleaseNotes.md | 3 ++ 6 files changed, 42 insertions(+), 35 deletions(-) diff --git a/Code/ForceField/UFF/Params.cpp b/Code/ForceField/UFF/Params.cpp index 89bd24457..b84968159 100644 --- a/Code/ForceField/UFF/Params.cpp +++ b/Code/ForceField/UFF/Params.cpp @@ -1,6 +1,5 @@ -// $Id$ // -// Copyright (C) 2004-2006 Rational Discovery LLC +// Copyright (C) 2004-2021 Greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -21,20 +20,28 @@ #include #include -typedef boost::tokenizer > tokenizer; +typedef boost::tokenizer> tokenizer; + +#include +#include +#include +#include +#include namespace ForceFields { namespace UFF { -class std::unique_ptr ParamCollection::ds_instance = nullptr; - extern const std::string defaultParamData; -ParamCollection *ParamCollection::getParams(const std::string ¶mData) { - if (ds_instance == nullptr || !paramData.empty()) { - ds_instance.reset(new ParamCollection(paramData)); - } - return ds_instance.get(); +typedef boost::flyweight< + boost::flyweights::key_value, + boost::flyweights::no_tracking> + param_flyweight; + +const ParamCollection *ParamCollection::getParams( + const std::string ¶mData) { + const ParamCollection *res = &(param_flyweight(paramData).get()); + return res; } ParamCollection::ParamCollection(std::string paramData) { @@ -83,6 +90,8 @@ ParamCollection::ParamCollection(std::string paramData) { inLine = RDKit::getLine(inStream); } } + +// clang-format off const std::string defaultParamData = "#Atom r1 theta0 x1 D1 zeta Z1 Vi Uj " "Xi Hard Radius\n" @@ -340,5 +349,7 @@ const std::string defaultParamData = "3.475 3.175 1.9\n" "Lw6+3 1.698 90 3.236 0.011 12 3.9 0 0 " "3.5 3.2 1.9\n"; +// clang-format on + } // end of namespace UFF } // end of namespace ForceFields diff --git a/Code/ForceField/UFF/Params.h b/Code/ForceField/UFF/Params.h index ea4de7f3c..e5b64ea77 100644 --- a/Code/ForceField/UFF/Params.h +++ b/Code/ForceField/UFF/Params.h @@ -1,5 +1,5 @@ // -// Copyright (C) 2004-2006 Rational Discovery LLC +// Copyright (C) 2004-2021 Greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -8,8 +8,8 @@ // of the RDKit source tree. // #include -#ifndef __RD_UFFPARAMS_H__ -#define __RD_UFFPARAMS_H__ +#ifndef RD_UFFPARAMS_H +#define RD_UFFPARAMS_H #include #include @@ -129,7 +129,7 @@ class RDKIT_FORCEFIELD_EXPORT ParamCollection { - if \c paramData is supplied, a new singleton will be instantiated. The current instantiation (if there is one) will be deleted. */ - static ParamCollection *getParams(const std::string ¶mData = ""); + static const ParamCollection *getParams(const std::string ¶mData = ""); //! Looks up the parameters for a particular UFF key and returns them. /*! \return a pointer to the AtomicParams object, NULL on failure. @@ -143,10 +143,9 @@ class RDKIT_FORCEFIELD_EXPORT ParamCollection { return nullptr; } - private: - //! to force this to be a singleton, the constructor must be private ParamCollection(std::string paramData); - static class std::unique_ptr ds_instance; //!< the singleton + + private: std::map d_params; //!< the parameter map }; } // namespace UFF diff --git a/Code/ForceField/UFF/testUFFForceField.cpp b/Code/ForceField/UFF/testUFFForceField.cpp index 1692589db..98dd64e98 100644 --- a/Code/ForceField/UFF/testUFFForceField.cpp +++ b/Code/ForceField/UFF/testUFFForceField.cpp @@ -1,6 +1,5 @@ -// $Id$ // -// Copyright (C) 2004-2008 Greg Landrum and Rational Discovery LLC +// Copyright (C) 2004-2021 Greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -936,8 +935,7 @@ void testUFFParams() { std::cerr << "-------------------------------------" << std::endl; std::cerr << " Test UFF Parameter objects" << std::endl; - ForceFields::UFF::ParamCollection *params = - ForceFields::UFF::ParamCollection::getParams(); + auto params = ForceFields::UFF::ParamCollection::getParams(); TEST_ASSERT(params); const ForceFields::UFF::AtomicParams *ptr; @@ -975,8 +973,7 @@ void testUFF8() { ps.push_back(&p5); ps.push_back(&p6); - ForceFields::UFF::ParamCollection *params = - ForceFields::UFF::ParamCollection::getParams(); + auto params = ForceFields::UFF::ParamCollection::getParams(); const ForceFields::UFF::AtomicParams *param1, *param2; // C_2 (sp2 carbon): @@ -1116,8 +1113,7 @@ void testUFFTorsionConflict() { ps.push_back(&p6); ps.push_back(&p7); - ForceFields::UFF::ParamCollection *params = - ForceFields::UFF::ParamCollection::getParams(); + auto params = ForceFields::UFF::ParamCollection::getParams(); const ForceFields::UFF::AtomicParams *param1, *param2, *param3; // C_2 (sp2 carbon): diff --git a/Code/GraphMol/ForceFieldHelpers/UFF/AtomTyper.cpp b/Code/GraphMol/ForceFieldHelpers/UFF/AtomTyper.cpp index 4af3e1fd8..1ff304d5d 100644 --- a/Code/GraphMol/ForceFieldHelpers/UFF/AtomTyper.cpp +++ b/Code/GraphMol/ForceFieldHelpers/UFF/AtomTyper.cpp @@ -1,6 +1,5 @@ -// $Id$ // -// Copyright (C) 2004-2006 Rational Discovery LLC +// Copyright (C) 2004-2021 Greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -393,7 +392,7 @@ std::string getAtomLabel(const Atom *atom) { std::pair getAtomTypes(const ROMol &mol, const std::string &) { bool foundAll = true; - ParamCollection *params = ParamCollection::getParams(); + auto params = ParamCollection::getParams(); AtomicParamVect paramVect; paramVect.resize(mol.getNumAtoms()); @@ -420,7 +419,7 @@ std::pair getAtomTypes(const ROMol &mol, bool getUFFBondStretchParams(const ROMol &mol, unsigned int idx1, unsigned int idx2, UFFBond &uffBondStretchParams) { - ParamCollection *params = ParamCollection::getParams(); + auto params = ParamCollection::getParams(); unsigned int idx[2] = {idx1, idx2}; AtomicParamVect paramVect(2); unsigned int i; @@ -445,7 +444,7 @@ bool getUFFBondStretchParams(const ROMol &mol, unsigned int idx1, bool getUFFAngleBendParams(const ROMol &mol, unsigned int idx1, unsigned int idx2, unsigned int idx3, UFFAngle &uffAngleBendParams) { - ParamCollection *params = ParamCollection::getParams(); + auto params = ParamCollection::getParams(); unsigned int idx[3] = {idx1, idx2, idx3}; AtomicParamVect paramVect(3); unsigned int i; @@ -477,7 +476,7 @@ bool getUFFAngleBendParams(const ROMol &mol, unsigned int idx1, bool getUFFTorsionParams(const ROMol &mol, unsigned int idx1, unsigned int idx2, unsigned int idx3, unsigned int idx4, UFFTor &uffTorsionParams) { - ParamCollection *params = ParamCollection::getParams(); + auto params = ParamCollection::getParams(); unsigned int idx[4] = {idx1, idx2, idx3, idx4}; AtomicParamVect paramVect(2); unsigned int i; @@ -599,7 +598,7 @@ bool getUFFInversionParams(const ROMol &mol, unsigned int idx1, bool getUFFVdWParams(const ROMol &mol, unsigned int idx1, unsigned int idx2, UFFVdW &uffVdWParams) { bool res = true; - ParamCollection *params = ParamCollection::getParams(); + auto params = ParamCollection::getParams(); unsigned int idx[2] = {idx1, idx2}; AtomicParamVect paramVect(2); unsigned int i; diff --git a/Code/GraphMol/ForceFieldHelpers/UFF/testUFFHelpers.cpp b/Code/GraphMol/ForceFieldHelpers/UFF/testUFFHelpers.cpp index f66677e83..a22c4734b 100644 --- a/Code/GraphMol/ForceFieldHelpers/UFF/testUFFHelpers.cpp +++ b/Code/GraphMol/ForceFieldHelpers/UFF/testUFFHelpers.cpp @@ -1,5 +1,5 @@ // -// Copyright (C) 2004-2018 Greg Landrum and Rational Discovery LLC +// Copyright (C) 2004-2021 Greg Landrum and other RDKit contributors // // @@ All Rights Reserved @@ // This file is part of the RDKit. @@ -1387,8 +1387,7 @@ void testGitHubIssue613() { TEST_ASSERT(foundAll); TEST_ASSERT(types.size() == mol->getNumAtoms()); - ForceFields::UFF::ParamCollection *params = - ForceFields::UFF::ParamCollection::getParams(); + auto params = ForceFields::UFF::ParamCollection::getParams(); const ForceFields::UFF::AtomicParams *ap = (*params)("Eu6+3"); TEST_ASSERT(ap); TEST_ASSERT(ap->r1 == types[0]->r1); diff --git a/ReleaseNotes.md b/ReleaseNotes.md index f5606cac9..f93e818a6 100644 --- a/ReleaseNotes.md +++ b/ReleaseNotes.md @@ -9,6 +9,9 @@ valid molecules. - Molecule names in SMILES and SMARTS are now parsed by default. Previously they were ignored. +- The `getParams()` function for retrieving UFF parameters now returns a const + pointer instead of a standard pointer. This shouldn't affect the functionality + of any code since the only method of the class is also const. ## Code removed in this release: - The minimizeOnly option for coordgen has been removed.