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.

Sign up for GitHub

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

Jump to bottom

Unusual definition of isnan() and its implications #603

Closed
whitequark opened this issue May 10, 2020 · 1 comment
Closed

Unusual definition of isnan() and its implications #603

whitequark opened this issue May 10, 2020 · 1 comment
Labels
internals question
Milestone
3.0

Comments

@whitequark
Copy link
Contributor

Commit ea6db67 added an unusual isnan macro:

#define isnan(x) (((x) != (x)) || (x > 1e11) || (x < -1e11))

Commit 8bc322e adds a preprocessor guard that looks like it would cause the isnan function from math.h to be preferred, but doesn't actually do that on many platforms, e.g. glibc:

#ifndef isnan
#   define isnan(x) (((x) != (x)) || (x > 1e11) || (x < -1e11))
#endif

@jwesthues I'm not really sure what's the intent here--is it to check for NaN and infinity, or is it really a comparison with VERY_POSITIVE and VERY_NEGATIVE? Regardless, we need a better name for the function that doesn't clash with math.h, and to get rid of the misleading preprocessor junk.
@whitequark whitequark added this to the 3.0 milestone May 10, 2020
@jwesthues
Copy link
Member

Yeah, clearly not good. This is used for stuff like testing whether numerical algorithms converged to anything reasonable, so actual standard isnan() is definitely not what we want--we definitely want to return true for ±Inf, and for implausibly large or small normal numbers too. I'd keep the definition but rename, to IsReasonable() or something.

@phkahler phkahler mentioned this issue May 11, 2020
Closed
whitequark added a commit that referenced this issue May 12, 2020
@whitequark
Rename our isnan() function to IsReasonable().
0be42a8 
Commit ea6db67 added an unusual isnan macro:

  #define isnan(x) (((x) != (x)) || (x > 1e11) || (x < -1e11))

Commit 8bc322e adds a preprocessor guard that looks like it would
cause the isnan function from math.h to be preferred, but doesn't
actually do that on many platforms, e.g. glibc:

  #ifndef isnan
  #   define isnan(x) (((x) != (x)) || (x > 1e11) || (x < -1e11))
  #endif

This commit renames our isnan() to make it clear that it differs
from the standard library operation, and makes it a function.

Fixes #603.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
internals question
Projects
None yet
Milestone
3.0
Development

Successfully merging a pull request may close this issue.

Renamed isnan() to IsReasonable phkahler/solvespace
2 participants
@whitequark @jwesthues