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

Div by zero fixes #204

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

Conversation

GlassBeaver
Copy link

Fixed three cases of division by zero.

You can use the following macro to get errors at runtime when a div by zero or other floating-point error occurs, like NaN or inf.
Need to enable it on every thread individually at thread creation (for a single-threaded app, just put it first thing in the main function). Ideally, you put this inside #if _DEBUG

// throw floating-point exceptions on any errors and NaN or inf
#define THROW_ON_FP_ERROR()
uint32_t origFP;
_controlfp_s(&origFP, 0, 0);
_controlfp_s(nullptr, origFP & ~(_EM_INVALID | _EM_ZERODIVIDE), _MCW_EM);

@Chlumsky
Copy link
Owner

What is wrong with division by zero? Subsequent conditions will detect inf & nan values, so I don't see the issue. Do you have any examples where this fixes a wrong result?

@GlassBeaver
Copy link
Author

In your particular functions, you might be able to get away with it but others using your library might be looking out for div by zero and other floating-point errors for various reasons. See above for the macro. In such a case, their programs won't get past the erroneous parts.

I'm not sure I understand why it's a problem to fix the div by zero errors right at the source.

If you're worried about performance, the branch predictor has a very good chance of predicting the additional branches (you can verify that in Intel vTune or AMD uProf) and if there are lots of div by zeroes then you're actually saving performance by doing an early exit, most notably in hasDiagonalArtifact() since you would skip all the calls to hasDiagonalArtifactInner()

@Chlumsky
Copy link
Owner

Because I don't see floating-point division by zero as an error to be fixed, it's just something that can happen.

@GlassBeaver
Copy link
Author

See my explanation above for why it's considered an error. Fixing it doesn't hurt the codebase, on the contrary it actually improves it. See above for the performance-related part if you're concerned about performance. If you're offended by my fix for whatever reason then reject the PR and be done with it. It's certainly not worth it to argue about this.

@Chlumsky
Copy link
Owner

I am not offended or concerned about performance, I am simply unconvinced by your argument.

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.

2 participants