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

Improve NAN handling in ccmath #912

Merged
merged 12 commits into from
Jan 9, 2023
Merged

Improve NAN handling in ccmath #912

merged 12 commits into from
Jan 9, 2023

Conversation

mborland
Copy link
Member

@mborland mborland commented Jan 7, 2023

No description provided.

Copy link
Collaborator

@pulver pulver left a comment

Choose a reason for hiding this comment

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

For the hypot(), log(), and sqrt() functions, shouldn't parameters with values of negative infinity result in NaN instead of negative infinity?

E.g. sqrt(-∞) = nan rather than -∞.

I know this behavior remains unchanged in this PR, but thought this might be an opportune time to fix.

@mborland
Copy link
Member Author

mborland commented Jan 7, 2023

For the hypot(), log(), and sqrt() functions, shouldn't parameters with values of negative infinity result in NaN instead of negative infinity?

E.g. sqrt(-∞) = nan rather than -∞.

I know this behavior remains unchanged in this PR, but thought this might be an opportune time to fix.

The language specification is +- infinity returns +- infinity. Since we fallback to the STL at runtime I didn't want the edge case handling to differ based on context.

@jzmaddock
Copy link
Collaborator

The language specification is +- infinity returns +- infinity. Since we fallback to the STL at runtime I didn't want the edge case handling to differ based on context.

C99 at least specifies that sqrt of a negative number is a domain error and yields a NaN, as things stand runtime sqrt(-INF) returns NaN and constexpr sqrt(-INF) returns +INF which looks just plain wrong?

@mborland
Copy link
Member Author

mborland commented Jan 7, 2023

The language specification is +- infinity returns +- infinity. Since we fallback to the STL at runtime I didn't want the edge case handling to differ based on context.

C99 at least specifies that sqrt of a negative number is a domain error and yields a NaN, as things stand runtime sqrt(-INF) returns NaN and constexpr sqrt(-INF) returns +INF which looks just plain wrong?

Ok for sqrt the standard only specifies +inf. Domain error is implementation defined so NaN would make sense.

@pulver
Copy link
Collaborator

pulver commented Jan 7, 2023

According to https://en.cppreference.com/w/c/numeric/math/logb

If arg is ±∞, +∞ is returned

I haven't tested it, but inspecting the code it appears that for the constexpr version

logb(-∞) = -∞

Shouldn't it return +∞?

@mborland
Copy link
Member Author

mborland commented Jan 7, 2023

According to https://en.cppreference.com/w/c/numeric/math/logb

If arg is ±∞, +∞ is returned

I haven't tested it, but inspecting the code it appears that for the constexpr version

logb(-∞) = -∞

Shouldn't it return +∞?

Yes; the change is wrong.

@mborland
Copy link
Member Author

mborland commented Jan 8, 2023

@pulver Can you do one last dummy check for me? I think this is good now and addresses your sqrt(-∞) = nan.

Copy link
Collaborator

@pulver pulver left a comment

Choose a reason for hiding this comment

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

For hypot(), it looks like

hypot(-∞,1) = -∞

but should return +∞. Same when x and y are swapped, based on hypot:

if one of the arguments is ±∞, hypot(x,y) returns +∞ even if the other argument is NaN

Copy link
Collaborator

@pulver pulver left a comment

Choose a reason for hiding this comment

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

LGTM

@mborland mborland merged commit aaa8684 into boostorg:develop Jan 9, 2023
@mborland mborland deleted the ccmath branch January 9, 2023 15:12
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