Various improvements to FloatCore #41
Conversation
Formerly changed on the next branch, part of rust-num/num#319.
There was a problem hiding this comment.
The changes look good to me. I have a few comments:
- What is the motivation behind adding
NumCasttoFloatCore? Just symmetry withFloat? I think the traits are kind of orthogonal. It seems like you needNumCastfor the implementation ofround. - Should we use associated constants rather than static methods? Would that raise our minimal supported Rust version?
- Are the implementations of the rounding functions canonical? I don't know enough about the properties of floats to vouch for their correctness. It seems like tests for them are missing?
src/float.rs
Outdated
| self = self.recip(); | ||
| } | ||
| // It should always be possible to convert a positive `i32` to a `usize`. | ||
| super::pow(self, exp.to_usize().unwrap()) | ||
| super::pow(self, exp as u32 as usize) |
There was a problem hiding this comment.
To handle i32::MIN. See above that I switched it to wrapping_neg, which will leave MIN the same. We want that to become an unsigned positive 0x80000000, but to_usize() would just fail.
There was a problem hiding this comment.
I compromised -- added a comment why the as u32 is needed, but went back to using to_usize() so we won't be surprised here by a platform with 16-bit usize (although that will panic).
| assert!((FloatCore::to_degrees(rad) - deg).abs() < 1e-6); | ||
| assert!((FloatCore::to_radians(deg) - rad).abs() < 1e-6); | ||
| assert!((FloatCore::to_degrees(rad) - deg).abs() < 1e-5); | ||
| assert!((FloatCore::to_radians(deg) - rad).abs() < 1e-5); |
There was a problem hiding this comment.
After I made it forward to std's f32::to_degrees, it started failing on nightly. I suspect rust-lang/rust#47919, which tried to be more accurate, actually made things less accurate for these inputs, but I haven't investigated yet. I will probably file an upstream bug.
src/float.rs
Outdated
| @@ -164,14 +505,54 @@ pub trait FloatCore: Num + NumCast + Neg<Output = Self> + PartialOrd + Copy { | |||
| } | |||
|
|
|||
| /// Returns `true` if `self` is positive, including `+0.0` and | |||
| /// `FloatCore::infinity()`. | |||
| /// `FloatCore::infinity()`, and with newer versions of Rust | |||
| /// even `FloatCore::nan()`. | |||
There was a problem hiding this comment.
Can we be explicit about the Rust version?
There was a problem hiding this comment.
Yeah, I'll look that up.
There was a problem hiding this comment.
Changed the comment to mention that this was Rust 1.20.
For symmetry, yes. I thought it made sense to keep I did end up using
It would raise it a lot -- associated consts were only made stable in 1.20.
I implemented them independently based on their description and observed behavior. I'm fairly confident in them, but not 100% sure. They do have doctests where I tried to exercise interesting cases, and note this gets tested both with and without forwarding to |
|
I'm going to merge this so we can publish. The rounding parts that I'm less confident about can always be bug-fixed later. Thanks for reviewing! bors r=vks |
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.
Build succeeded |
FloatandRealuse this to compact their implementations.FloatCorenow forwardsstdimplementations when possible.FloatCorenow requiresNumCast, like `Float does.FloatCore:min_value()->f64::MINfloor,ceil,round,trunc,fractinteger_decodematchingFloat'sFloatCore(assertion failed: !nan.is_sign_positive() && !nan.is_sign_negative() num#312, IEEE754 non-conformance: is_sign_negative does not apply on NANs rust-lang/rust#42425)FloatCore::powiexponent negation.FloatCoremethods.