Skip to content

Conversation

@djb-rwth
Copy link
Collaborator

@djb-rwth djb-rwth commented Dec 11, 2025

This PR includes:

  • 40+ Coverity Scan high priority fixes; the fixes have been confirmed by Coverity Scan
  • Transferring all remaining doxygen comments from .c files to .h files

Due to the number of fixes, it requires detailed PubChem testing.
More medium priority fixes to follow, but posting these now in a timely manner as agreed.

@djb-rwth djb-rwth self-assigned this Dec 11, 2025
@djb-rwth djb-rwth changed the title Coverity Scan fixes Coverity Scan fixes #1 Dec 11, 2025
Copy link
Collaborator

@JanCBrammer JanCBrammer left a comment

Choose a reason for hiding this comment

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

Note to myself and other reviewers: since the PR contains mostly style changes, a lot of effort goes into visually searching the diffs for semantic changes (i.e., the fixes). To view only the semantic changes check the "Hide whitespace" box in the review configuration (https://github.blog/changelog/2021-10-14-hiding-whitespace-is-now-remembered-for-each-pull-request/).

@djb-rwth, some files contain only style changes (e.g., mol2atom.c). Could you remove those from the PR? I feel like it's better to limit the style changes to files that are touched anyway for fixing. Otherwise the PRs / diffs get impractical to review.

@djb-rwth
Copy link
Collaborator Author

@djb-rwth, some files contain only style changes (e.g., mol2atom.c). Could you remove those from the PR? I feel like it's better to limit the style changes to files that are touched anyway for fixing. Otherwise the PRs / diffs get impractical to review.

Done.

@djb-rwth
Copy link
Collaborator Author

Hi @JanCBrammer,
Works now at my end, but the tests are a dud -- please look into this.

@JanCBrammer
Copy link
Collaborator

@djb-rwth, we were asserting that MakeINCHIFromMolfileText dies. Now that you've fixed MakeINCHIFromMolfileText, the test fails because it no longer dies. I've changed the assertion to check for successful InChI computation (see my latest commit), which makes the test pass.

@JanCBrammer
Copy link
Collaborator

@djb-rwth, could you merge this PR?

@djb-rwth djb-rwth merged commit 20f1f1c into dev Dec 19, 2025
2 checks passed
@djb-rwth
Copy link
Collaborator Author

@djb-rwth, we were asserting that MakeINCHIFromMolfileText dies. Now that you've fixed MakeINCHIFromMolfileText, the test fails because it no longer dies. I've changed the assertion to check for successful InChI computation (see my latest commit), which makes the test pass.

Hi @JanCBrammer,
Thanks for your quick action regarding this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants