Tautomer Query copy constructor is shallow not deep causing segfaults in destructor

[bp-kelley]

Failing Test (for tautomerQueryTestCatch)

TEST_CASE("TAUTOMERQUERY_COPY_CONSTRUCTOR") {
  auto mol = "c1ccccc1"_smiles;
  auto tautomerQuery =
    std::unique_ptr<TautomerQuery>(TautomerQuery::fromMol(*mol));
  auto tautomerQueryCopyConstructed = std::unique_ptr<TautomerQuery>(new TautomerQuery(*tautomerQuery));
}

[bp-kelley]

@jones-gareth @greglandrum

It appears that the template molecule should be a shared ptr as well:

  const std::vector<ROMOL_SPTR> d_tautomers;
  // Template query for substructure search
  const ROMol *const d_templateMolecule;

should be

  const std::vector<ROMOL_SPTR> d_tautomers;
  // Template query for substructure search
  const boost::shared_ptr<const ROMol*> d_templateMolecule;

However, I remember some compilers not liking shared_ptrs to const pointers.
This does have an issue with threading with TautomerQueries though if we are doing recursive queries and really need a full copy. I would like a way to do a proper deep copy.

[greglandrum]

Another approach to fixing this which solves the thread safety problem would be keep d_templateMolecule as a naked pointer but to explicitly copy the molecule in the copy constructor for TautomerQuery.

[bp-kelley]

Are the tautomers used as well during the search?

[greglandrum]

ah, right. It's the tautomers that are used during the query, not the template molecule.
I still think it makes sense to do a deep copy of the whole thing in the copy ctor.

[bp-kelley]

I agree that a deep copy should be made, I’ll update the pr.

…

---- Brian Kelley

On Feb 17, 2021, at 2:28 PM, Greg Landrum ***@***.***> wrote:  ah, right. It's the tautomers that are used during the query, not the template molecule. I still think it makes sense to do a deep copy of the whole thing in the copy ctor. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.