Fix ToPrimitive for f64 -> f32 conversion. #180
Conversation
|
I guess this is stopping because of the ongoing trait refactoring? That's ok, I think splitting is a good idea. |
|
Yeah, sorry I didn't say so. I'm holding off on approving more PRs until
that goes through.
|
|
☔ The latest upstream changes (presumably #164) made this pull request unmergeable. Please resolve the merge conflicts. |
bluss
force-pushed
the
fix-toprimitive-float
branch
from
fc5f6c4 to
190e89c
Compare
|
Updated. This is a breaking (behavior, not compile) change but also a bug fix. |
|
The effect of the bug was that it always return Some for floats. Maybe that's a bad thing to change without a new version? The current code is just nonsensical and we could change it into just the cast without the |
|
Hm ok it's |
|
I think it's safe to classify this as a pure bugfix, not a breaking change, as the intent was clearly there to have a range check. Otherwise, we'll never be able to change anything! (ref https://xkcd.com/1172/) However, there are some strange edge cases here. If I'm reading correctly, f32->f64 inf will succeed, but either f32->f32 or f64->f64 will be None. I think it's reasonable to expect inf and NaN to convert all around, but then shouldn't large f64 just be f32 inf anyway? |
|
(nevermind, comparing sizes |
|
I had to do research. Now we have even better backing for changing the behavior. Casting a too large f64 to f32 Now this isn't fully num's concern to solve, core Rust will have to solve this and I think it's pretty well known (the trouble with |
|
Can you make it so +/-inf and NaN carry over from f64 to f32? The range check could decide to manually create inf for out of range values, dodging the undefined result. But maybe None is better so there's a signal that it didn't really convert. (And then float->int is completely unchecked... #135 was working on that.) |
|
Yes, I will come back to it tomorrow. |
It should use the destination type and not the source type to check if the conversion would be to a value that's in range. NOTE: A finite f64 value that is larger than the f32 value range now produces None when converted to f32 with ToPrimitive. Previously, too large f64 values would produce inf f32 values. This `as` cast has an undefined result and was not specified to always produce for example `inf`. The conversion preserves nan/+-inf specifically.
bluss
force-pushed
the
fix-toprimitive-float
branch
from
190e89c to
acde249
Compare
|
Updated commit & PR message. It now preserves nan/inf (using !f32::is_finite()) |
|
⚡ Test exempted - status |
Fix ToPrimitive for f64 -> f32 conversion. Fix ToPrimitive for f64 -> f32 conversion. It should use the destination type and not the source type to check if the conversion would be to a value that's in range. NOTE: A finite f64 value that is larger than the f32 value range now produces None when converted to f32 with ToPrimitive. Previously, too large f64 values would produce inf f32 values. This `as` cast has an undefined result and was not specified to always produce for example `inf`. The conversion preserves nan/+-inf specifically.
Fix ToPrimitive for f64 -> f32 conversion.
It should use the destination type and not the source type to check if
the conversion would be to a value that's in range.
NOTE: A finite f64 value that is larger than the f32 value range now produces
None when converted to f32 with ToPrimitive.
Previously, too large f64 values would produce inf f32 values. This
ascast has an undefined result and was not specified to always produce for
example
inf.The conversion preserves nan/+-inf specifically.