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

Tidy up #2091

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Tidy up #2091

wants to merge 1 commit into from

Conversation

e-kwsm
Copy link
Contributor

@e-kwsm e-kwsm commented Nov 21, 2019

@ghutchis
Copy link
Member

Why? For many reasons, we removed the math checks because they're very slow.

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Nov 24, 2019

For 6af03f1, an input is not always normalizable and exception is thrown (#2088).

For 80468c4, the default release type is Release and NDEBUG is defined (e.g. L444), therefore performance is not harmed.
For vector3::operator[], probably switch with default would be better.

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Nov 24, 2019

switch is used; I hope performance is not worsen.

@ghutchis
Copy link
Member

ghutchis commented Dec 2, 2019

I've had this debate before - if there's a crash, the problem is in the calling code (e.g., GetNewBondVector in your linked example).

This code is called a lot in tight loops. Many builds are RelWithDebInfo which would still enable your checks.

@e-kwsm e-kwsm changed the title Check if a vector can be normalized (fix #2088) Tidy up Dec 5, 2019
@e-kwsm
Copy link
Contributor Author

e-kwsm commented Dec 9, 2019

Change about NDEBUG is reverted.

return _vz;
default:
cerr << "ERROR in OpenBabel::vector3::operator[]\n"
<< "The method has been called with an illegal index i=" << i << ".\n"
<< "Please contact the author of the offending program immediately." << endl;

This comment was marked as resolved.

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.

None yet

3 participants