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

test that shows that isNaN call is not reliable #1135

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

pjfanning
Copy link
Member

@cowtowncoder
Copy link
Member

I think it would make more sense to create a failing test? As is, test verifies what we think is an incorrect behavior, right?

But there is an interesting question here wrt if and how to allow checking of "out of range for Double" case, as opposed to NaN.

@pjfanning
Copy link
Member Author

pjfanning commented Nov 7, 2023

The test passes. I think it is useful as is. There may be merit changing the behaviour of 'isNan' method or adding a new method that checks if the number if Positive or Negative Infinity or NaN.

@cowtowncoder
Copy link
Member

I just conceptually dislike tests that verify Wrong behavior... but I can see usefulness.

As to adding more methods, yeah I don't know. Problem is that for Double over-/underflow the answer depends on target: for BigDecimal they are finite numbers, for double/Double infinite.

@cowtowncoder
Copy link
Member

@pjfanning I think I agree with this in general. But since it is very unlikely we'll try to fix this for 2.16 (due to change in behavior someone might rely), could you re-base/re-create this for 2.17?

cowtowncoder added a commit that referenced this pull request Nov 29, 2023
@pjfanning pjfanning changed the base branch from 2.16 to 2.17 November 29, 2023 20:54
@pjfanning
Copy link
Member Author

@cowtowncoder rebased

@cowtowncoder cowtowncoder merged commit 6836061 into FasterXML:2.17 Nov 29, 2023
5 checks passed
@pjfanning pjfanning deleted the nan-test branch November 30, 2023 08:06
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