-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Move NaN tests to floats/mod.rs #143396
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?
Move NaN tests to floats/mod.rs #143396
Conversation
f128: #[cfg(any(miri, target_has_reliable_f128_math))], | ||
}, | ||
test<Float> { | ||
use std::num::FpCategory as Fp; |
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.
The as Fp
is copied from the original tests, but I don't actually see much reason for it.
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.
Can you move it to the top of this file instead? The short form makes a bit more sense with the other infinite/finite/etc tests that also use it. (Not that we need to shorten it but 🤷♂ )
assert!(!nan.is_normal()); | ||
assert!(nan.is_sign_positive()); | ||
assert!(!nan.is_sign_negative()); | ||
assert!(matches!(nan.classify(), Fp::Nan)); |
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.
This is changed from an assert_eq
to work in a const context.
I just opened that issue to track this, but for review I think it'd be better to |
f16: #[cfg(any(miri, target_has_reliable_f16_math))], | ||
f128: #[cfg(any(miri, target_has_reliable_f128_math))], |
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.
Sorry, noticed something; could you use target_has_reliable_f{16,128}
rather than the _math
versions? _math
is only for anything that calls libm
functions, which these don't.
Mind also moving this above the other float_test
instances? Might as well keep things ordered setup-> basic tests -> math tests, like the other test files.
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.
(just updating the status)
This PR moves NaN tests to
floats/mod.rs
, as discussed in #141726. Since this is my first PR against Rust, I'm keeping it as small as possible, but I intend to work my way through the remaining tests and can do that work in this PR if that's preferable.r? RalfJung