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

Implement conversion traits for primitive float types #29129

Merged
merged 2 commits into from
Oct 29, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions src/libcore/num/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1473,14 +1473,14 @@ impl fmt::Display for ParseIntError {

pub use num::dec2flt::ParseFloatError;

// Conversion traits for primitive integer types
// Conversion traits for primitive integer and float types
// Conversions T -> T are covered by a blanket impl and therefore excluded
// Some conversions from and to usize/isize are not implemented due to portability concerns
macro_rules! impl_from {
($Small: ty, $Large: ty) => {
#[stable(feature = "lossless_int_conv", since = "1.5.0")]
#[stable(feature = "lossless_prim_conv", since = "1.5.0")]
impl From<$Small> for $Large {
#[stable(feature = "lossless_int_conv", since = "1.5.0")]
#[stable(feature = "lossless_prim_conv", since = "1.5.0")]
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess if this isn't merged before 1.5 branches, I'll need a distinct macro and stable tag, right?

Copy link
Member

Choose a reason for hiding this comment

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

Although this will technically land during 1.6, I think these are fine here. We don't actually read these since versions anywhere, and it'd just be a pain to separate out this macro for 1.5 stable and 1.6 stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, this PR is incorrectly listed in RELEASES.md under 1.5, possibly due to this confusion between the feature tag and the time of the actual merge. (I was wondering why my PR was listed but I wasn't in the contributor list...)

@brson -- need to tag this somehow so you can remember it for 1.6? It seems like "relnotes" needs to be version-specific.

#[inline]
fn from(small: $Small) -> $Large {
small as $Large
Expand Down Expand Up @@ -1514,3 +1514,24 @@ impl_from! { u8, i64 }
impl_from! { u16, i32 }
impl_from! { u16, i64 }
impl_from! { u32, i64 }

// Note: integers can only be represented with full precision in a float if
// they fit in the significand, which is 24 bits in f32 and 53 bits in f64.
// Lossy float conversions are not implemented at this time.

// Signed -> Float
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment about the selection of types here, for future reference? (This is totally something I could imagine someone looking at and wondering about, especially since the details of floating point aren't in the front of everyone's head all the time.)

I.e. mention something about the precision (24 and 53 bits respectively) meaning these types being the only integers which can be represented losslessly in the float types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I just pushed a new comment for this.

impl_from! { i8, f32 }
impl_from! { i8, f64 }
impl_from! { i16, f32 }
impl_from! { i16, f64 }
impl_from! { i32, f64 }

// Unsigned -> Float
impl_from! { u8, f32 }
impl_from! { u8, f64 }
impl_from! { u16, f32 }
impl_from! { u16, f64 }
impl_from! { u32, f64 }

// Float -> Float
impl_from! { f32, f64 }
57 changes: 57 additions & 0 deletions src/libcoretest/num/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,4 +177,61 @@ mod tests {
test_impl_from! { test_u16i32, u16, i32 }
test_impl_from! { test_u16i64, u16, i64 }
test_impl_from! { test_u32i64, u32, i64 }

// Signed -> Float
test_impl_from! { test_i8f32, i8, f32 }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say conversions integer -> float feel... too suspicious for Into, even if they are lossless. I'd prefer to use more specialized methods/traits for this, at least for concrete types.
Do you have any example of generic code where this Into would be useful (I.e. a conversion from integer to float is required and it should be completely precise)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll elaborate. I have a criterion for Into/AsRef: Suppose you have a function fn f(arg: Into<U>) (or try!, it also uses From for implicit conversions). Would you want it to implicitly accept Ts? If yes, then implementing Into<U> for T is reasonable. All current implementations meet this criterion (except for impl From<u32> for Ipv4Addr). Would you want fn f(arg: Into<f64>) to accept u8 implicitly? Probably not, there's a good chance this u8 is not your desired number, but its index, for example. I.e. type safety prevents mistakes. On the other hand, would you want it to accept u64 with explicit conversion? Probably yes, because zero error requirement is not normally important for such conversions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for elaborating. I don't have a specific example already where this would be useful, but it's not hard to concoct one. Say log<F: Into<f64>>(self, base: F) -> f64, where you want the flexibility of float but the base is often integral. Yes, I think this could accept u8, but that's not really implicit since the function used Into, opting into that flexibility. The "type safety prevents mistakes" argument doesn't speak to me much, because you could say the same about even integral conversions, and again Into is an opt-in choice.

But I realize this is a pretty subjective thing to judge. Is there anyone else we should ping for an opinion about int->float? Do you at least agree with f32->f64?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anyone else we should ping for an opinion about int->float?

@rust-lang/libs ?
(My opinion doesn't matter much, I've posted it because #28921 was mentioned)

Do you at least agree with f32->f64?

Yes!

Copy link
Contributor

Choose a reason for hiding this comment

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

@cuviper

Say log<F: Into<f64>>(self, base: F) -> f64, where you want the flexibility of float but the base is often integral.

In this example F == u64 is good too, because precise conversion is not required, so Into would probably be too strict.

Copy link
Member Author

Choose a reason for hiding this comment

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

My opinion doesn't matter much [...]

My opinion carries no more weight than yours. :) I appreciate your input!

test_impl_from! { test_i8f64, i8, f64 }
test_impl_from! { test_i16f32, i16, f32 }
test_impl_from! { test_i16f64, i16, f64 }
test_impl_from! { test_i32f64, i32, f64 }

// Unsigned -> Float
test_impl_from! { test_u8f32, u8, f32 }
test_impl_from! { test_u8f64, u8, f64 }
test_impl_from! { test_u16f32, u16, f32 }
test_impl_from! { test_u16f64, u16, f64 }
test_impl_from! { test_u32f64, u32, f64 }

// Float -> Float
#[test]
fn test_f32f64() {
use core::f32;

let max: f64 = f32::MAX.into();
assert_eq!(max as f32, f32::MAX);
assert!(max.is_normal());

let min: f64 = f32::MIN.into();
assert_eq!(min as f32, f32::MIN);
assert!(min.is_normal());

let min_positive: f64 = f32::MIN_POSITIVE.into();
assert_eq!(min_positive as f32, f32::MIN_POSITIVE);
assert!(min_positive.is_normal());

let epsilon: f64 = f32::EPSILON.into();
assert_eq!(epsilon as f32, f32::EPSILON);
assert!(epsilon.is_normal());

let zero: f64 = (0.0f32).into();
assert_eq!(zero as f32, 0.0f32);
assert!(zero.is_sign_positive());

let neg_zero: f64 = (-0.0f32).into();
assert_eq!(neg_zero as f32, -0.0f32);
assert!(neg_zero.is_sign_negative());

let infinity: f64 = f32::INFINITY.into();
assert_eq!(infinity as f32, f32::INFINITY);
assert!(infinity.is_infinite());
assert!(infinity.is_sign_positive());

let neg_infinity: f64 = f32::NEG_INFINITY.into();
assert_eq!(neg_infinity as f32, f32::NEG_INFINITY);
assert!(neg_infinity.is_infinite());
assert!(neg_infinity.is_sign_negative());

let nan: f64 = f32::NAN.into();
assert!(nan.is_nan());
}
}