-
Notifications
You must be signed in to change notification settings - Fork 91
Remove NaN tests for Ord and Eq #316
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
base: master
Are you sure you want to change the base?
Conversation
I don't know if this is right exactly, these NaN operations are part of IEEE-754 which we probably use to make other assumptions about numbers too. |
I don't think we do other assumptions with regards to |
While these are more problematic due to operator inlining, looks like there are some more Ord/Eq tests? |
I've now removed all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyb Right now the instances/inlining situation is just incoherent. These tests aren't testing IEEE conformance so much as they are testing that the compiler inlines these operators. For example, use something other than prelude comparison operators, and these won't work!
I think if we have these tests anywhere they would need to be in the compiler with the JS backend because that's the only place this might make sense to validate.
Yeah, that's all fine, I'd completely forgotten about the previous discussion @anttih linked even though I participated. 😄 |
These tests rely on the
purs
inliner to provide correct behavior. With this change we leave theOrd
andEq
NaN
behavior undefined. A backend can choose how comparisons withNaN
work.Fixes #306