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

Document the hazard from signed zero #157

Closed
kpreid opened this issue Oct 3, 2024 · 1 comment · Fixed by #159
Closed

Document the hazard from signed zero #157

kpreid opened this issue Oct 3, 2024 · 1 comment · Fixed by #159

Comments

@kpreid
Copy link
Contributor

kpreid commented Oct 3, 2024

In f32 and f64, there is both a negative zero value, and a positive zero value. These values are considered equal by PartialEq, but have different results from operations such as division or reciprocal. This carries through to the NotNan and OrderedFloat wrappers:

{
    let a = NotNan::new(0.0f32).unwrap();
    let b = NotNan::new(-0.0f32).unwrap();
    let one = NotNan::new(1.0f32).unwrap();
    (a == b, one / a == one / b)    
}

The above code produces (true, false). This means that any code which uses NotNan or OrderedFloat as part of a cache key, or merely compares an old value to a new one (e.g. in a setter which has a side effect on change), may conclude that the result of two computations would be identical when they would not actually be.

This seems to me to be a hazard that would be worth mentioning in the documentation. It’s not a violation of Eq’s properties, but it is surprising and likely undesired in many applications.

It might also be useful to offer different wrappers which don't have this property, and consider the two zeros distinct. Or zero could be canonicalized on construction. Or (for specialized applications) all negative values rejected, making an even more limited but well-defined number type. (Any of these would do for most of the purposes I have in mind.)

@mbrubeck
Copy link
Collaborator

mbrubeck commented Oct 3, 2024

Good suggestion. If you’d like to open a PR to add appropriate docs, that would be great. (If not, I will try to do so at some point.)

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 a pull request may close this issue.

2 participants