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

IEEE754 non-conformance: is_sign_negative does not apply on NANs #42425

Closed
nagisa opened this issue Jun 4, 2017 · 3 comments
Closed

IEEE754 non-conformance: is_sign_negative does not apply on NANs #42425

nagisa opened this issue Jun 4, 2017 · 3 comments
Labels
C-bug Category: This is a bug. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Jun 4, 2017

IEEE754 says

― boolean isSignMinus(source)
isSignMinus(x) is true if and only if x has negative sign. isSignMinus applies to zeros and NaNs
as well.

However our is_sign_negative/is_sign_positive do not properly inspect the sign for NaNs.

@nagisa nagisa added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 4, 2017
@Thiez
Copy link
Contributor

Thiez commented Jun 4, 2017

This program:

fn main() {
    let nums = [::std::f64::NEG_INFINITY, -1.0, -0.0, ::std::f64::NAN, 0.0, 1.0, ::std::f64::INFINITY];
    for &num in &nums[..] {
        println!("Number: {}\tSign negative: {}\tSign positive: {}", num, num.is_sign_negative(), num.is_sign_positive());
    }
}

prints the following output when compiled with current master and nightly:

Number: -inf	Sign negative: true	Sign positive: false
Number: -1	Sign negative: true	Sign positive: false
Number: 0	Sign negative: true	Sign positive: false
Number: NaN	Sign negative: false	Sign positive: false
Number: 0	Sign negative: false	Sign positive: true
Number: 1	Sign negative: false	Sign positive: true
Number: inf	Sign negative: false	Sign positive: true

What would be the expected result according to the standard? I would expect something that isn't a number to be neither negative nor positive, so I think the current results are correct?

@nagisa
Copy link
Member Author

nagisa commented Jun 4, 2017

My reading of the spec suggests the answer should be equivalent to is_sign_negative = signbit(x) != 0, where signbit is non-zero if the binary representation has sign bit set.

E.g. Section 6.2.1:

When encoded, all NaNs have a sign bit and a pattern of bits necessary to identify the encoding as a NaN and which determines its kind (sNaN vs. qNaN).

alludes to NaNs having a sign bit. The description of isSignMinus seems to quite clearly signal towards the fact that for NaNs the implementation ought to be inspecting this sign bit.

You can set the sign of NaN in rust with -::std::f64::NAN as well.

@alexcrichton alexcrichton added P-medium Medium priority and removed I-nominated labels Jun 7, 2017
nagisa added a commit to nagisa/rust that referenced this issue Jun 22, 2017
bors added a commit that referenced this issue Jun 28, 2017
Fix NaN handling in is_sign_negative/positive

This would be my proposed fix for the #42425 provided we decide it is indeed a problem.

Note this would technically be a breaking change to a stable API. We might want to consider deprecating these methods and adding new ones.
@Mark-Simulacrum Mark-Simulacrum added C-bug Category: This is a bug. and removed I-wrong labels Jul 27, 2017
@hanna-kruppe
Copy link
Contributor

Isn't this fixed since #42431 was merged?

@nagisa nagisa closed this as completed Nov 20, 2017
bors bot added a commit to rust-num/num-traits that referenced this issue Mar 1, 2018
41: Various improvements to FloatCore r=vks a=cuviper

- New macros simplify forwarding method implementations.
  - `Float` and `Real` use this to compact their implementations.
  - `FloatCore` now forwards `std` implementations when possible.
- `FloatCore` now requires `NumCast`, like `Float does.
- New additions to `FloatCore`:
  - Constants like `min_value()` -> `f64::MIN`
  - Rounding methods `floor`, `ceil`, `round`, `trunc`, `fract`
  - `integer_decode` matching `Float`'s
- Fix NAN sign handling in `FloatCore` (rust-num/num#312, rust-lang/rust#42425)
- Fix overflow in `FloatCore::powi` exponent negation.
- Add doctests to all `FloatCore` methods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants