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

Fix float to int cast overflow #135

Closed
wants to merge 2 commits into from

Conversation

apopiak
Copy link

@apopiak apopiak commented Nov 25, 2015

  • add impl_to_primitive_float_to_int_or_uint macro with bounds checking
  • tests

fix #119

@apopiak
Copy link
Author

apopiak commented Nov 25, 2015

feel free to critique

@@ -1136,30 +1136,42 @@ macro_rules! impl_to_primitive_float_to_float {
)
}

macro_rules! impl_to_primitive_float_to_int_or_uint {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(bikeshed) I'd just call it ..._to_integer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I'll change it.

@cuviper
Copy link
Member

cuviper commented Dec 13, 2015

Sorry, I didn't notice you pushed an update for truncation. That looks good.

On the importance of edge cases, consider something like 2^64, which should be None for u64, but gives Some(0) instead, even with your new code.

assert_eq!(None, cast::<f64, u64>(64.0.exp2()));

The true maximum that can be converted is actually less than std::u64::MAX, since that value can't even be represented exactly in f64. I think it will be 0xFFFF_FFFF_FFFF_F800u64, where in f64 the 53-bit significand is maxed. Then f32 has only a 24-bit significand, so you get 0xFFFF_FF00_0000_0000u64 and 0xFFFF_FF00u32 edge cases.

If only next_after were in rust-stable, then we could directly write these limits, e.g. 64.0.exp2().next_after(0.0) and 32.0.exp2().next_after(0.0). But running these on nightly confirms the values I gave above. http://is.gd/xgCyG8

@apopiak
Copy link
Author

apopiak commented Dec 14, 2015

Hey, after his update I did some more testing and realized that my implementation had gaps (which you also found) and I didn't find the time to really dive in again and fix it completely. I'll have another look this week.
I wonder whether we could use the Integer and Rational traits? Wouldn't be very efficient, though, I guess.

@cuviper
Copy link
Member

cuviper commented Dec 14, 2015

If you don't want to hardcode significand-based MIN/MAX values, another possibility is to just check the round-trip. Something like:

let t = f.trunc();
let i = t as $ity;
if i as $fty == t { Some(i) } else { None }

But I'd still want those significand edge-cases tested, so we need those values anyway.

@homu
Copy link
Contributor

homu commented Feb 16, 2016

☔ The latest upstream changes (presumably ebed675) made this pull request unmergeable. Please resolve the merge conflicts.

@cuviper
Copy link
Member

cuviper commented Dec 19, 2017

We should still fix this, but this PR is long stagnant. See the new repo and issue here:
rust-num/num-traits#12

@cuviper cuviper closed this Dec 19, 2017
termoshtt pushed a commit to termoshtt/num-traits that referenced this pull request Aug 17, 2019
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 this pull request may close these issues.

Casting floats to integers does not check overflow
3 participants