Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some minor leaks #6029

Merged
merged 3 commits into from
Jan 31, 2023
Merged

Fix some minor leaks #6029

merged 3 commits into from
Jan 31, 2023

Conversation

ricrogz
Copy link
Contributor

@ricrogz ricrogz commented Jan 29, 2023

This fixes some minor leaks I found running asan on the tests:

  • Code/GraphMol/MolStandardize/test1.cpp: just one Mol that wasn't being deallocated.
  • Code/GraphMol/Fingerprints/FingerprintGenerator.cpp: atomInvariants and bondInvariants were not being cleaned up when something fails in downstream code.
  • Code/GraphMol/ROMol.cpp: ROMol::load() is not a constructor, so it can only be called on already initialized mols, so when we call initMol() (I assume that to reset properties and ring info, dp_ringInfo is already allocated, and leaks.

Copy link
Member

@greglandrum greglandrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@greglandrum greglandrum added this to the 2022_09_5 milestone Jan 31, 2023
@greglandrum
Copy link
Member

I will try to incorporate this into the next patch release too, but that may not completely work due to the extensive changes in the fingerprint generator code. Since these are minor problems, that won't be a giant tragedy

@greglandrum greglandrum merged commit 0e7e44c into rdkit:master Jan 31, 2023
@ricrogz ricrogz deleted the fix_leaks branch January 31, 2023 16:52
@ricrogz
Copy link
Contributor Author

ricrogz commented Jan 31, 2023

I will try to incorporate this into the next patch release too, but that may not completely work due to the extensive changes in the fingerprint generator code. Since these are minor problems, that won't be a giant tragedy

Sounds good to me.

greglandrum pushed a commit that referenced this pull request Feb 23, 2023
* fix deserializing leaks ringinfo

* fix leak in FingerprintGenerator

* fix leak in MolStandardize test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants