From d968efbc76639a89ae649c377375afdfb2686dd3 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Wed, 20 Jun 2018 13:05:03 -0700 Subject: [PATCH] Avoid `as` casts in default FromPrimitive methods Particularly, the default `from_f64` used `n as i64`, which has undefined behavior on overflow, kind of defeating the purpose here. Now we use a checked `to_i64()` for this, and even try `to_u64()` as a fallback for completeness. (All of the primitive implementations already do better, at least.) --- src/cast.rs | 25 +++++++++++++++---------- tests/cast.rs | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/cast.rs b/src/cast.rs index a8c8484..eb1fa70 100644 --- a/src/cast.rs +++ b/src/cast.rs @@ -355,28 +355,28 @@ pub trait FromPrimitive: Sized { /// value cannot be represented by this value, the `None` is returned. #[inline] fn from_isize(n: isize) -> Option { - FromPrimitive::from_i64(n as i64) + n.to_i64().and_then(FromPrimitive::from_i64) } /// Convert an `i8` to return an optional value of this type. If the /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_i8(n: i8) -> Option { - FromPrimitive::from_i64(n as i64) + FromPrimitive::from_i64(From::from(n)) } /// Convert an `i16` to return an optional value of this type. If the /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_i16(n: i16) -> Option { - FromPrimitive::from_i64(n as i64) + FromPrimitive::from_i64(From::from(n)) } /// Convert an `i32` to return an optional value of this type. If the /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_i32(n: i32) -> Option { - FromPrimitive::from_i64(n as i64) + FromPrimitive::from_i64(From::from(n)) } /// Convert an `i64` to return an optional value of this type. If the @@ -400,28 +400,28 @@ pub trait FromPrimitive: Sized { /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_usize(n: usize) -> Option { - FromPrimitive::from_u64(n as u64) + n.to_u64().and_then(FromPrimitive::from_u64) } /// Convert an `u8` to return an optional value of this type. If the /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_u8(n: u8) -> Option { - FromPrimitive::from_u64(n as u64) + FromPrimitive::from_u64(From::from(n)) } /// Convert an `u16` to return an optional value of this type. If the /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_u16(n: u16) -> Option { - FromPrimitive::from_u64(n as u64) + FromPrimitive::from_u64(From::from(n)) } /// Convert an `u32` to return an optional value of this type. If the /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_u32(n: u32) -> Option { - FromPrimitive::from_u64(n as u64) + FromPrimitive::from_u64(From::from(n)) } /// Convert an `u64` to return an optional value of this type. If the @@ -445,14 +445,17 @@ pub trait FromPrimitive: Sized { /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_f32(n: f32) -> Option { - FromPrimitive::from_f64(n as f64) + FromPrimitive::from_f64(From::from(n)) } /// Convert a `f64` to return an optional value of this type. If the /// type cannot be represented by this value, the `None` is returned. #[inline] fn from_f64(n: f64) -> Option { - FromPrimitive::from_i64(n as i64) + match n.to_i64() { + Some(i) => FromPrimitive::from_i64(i), + None => n.to_u64().and_then(FromPrimitive::from_u64), + } } } @@ -460,6 +463,7 @@ macro_rules! impl_from_primitive { ($T:ty, $to_ty:ident) => ( #[allow(deprecated)] impl FromPrimitive for $T { + #[inline] fn from_isize(n: isize) -> Option<$T> { n.$to_ty() } #[inline] fn from_i8(n: i8) -> Option<$T> { n.$to_ty() } #[inline] fn from_i16(n: i16) -> Option<$T> { n.$to_ty() } #[inline] fn from_i32(n: i32) -> Option<$T> { n.$to_ty() } @@ -467,6 +471,7 @@ macro_rules! impl_from_primitive { #[cfg(has_i128)] #[inline] fn from_i128(n: i128) -> Option<$T> { n.$to_ty() } + #[inline] fn from_usize(n: usize) -> Option<$T> { n.$to_ty() } #[inline] fn from_u8(n: u8) -> Option<$T> { n.$to_ty() } #[inline] fn from_u16(n: u16) -> Option<$T> { n.$to_ty() } #[inline] fn from_u32(n: u32) -> Option<$T> { n.$to_ty() } diff --git a/tests/cast.rs b/tests/cast.rs index edac185..fd248de 100644 --- a/tests/cast.rs +++ b/tests/cast.rs @@ -9,6 +9,7 @@ extern crate std; extern crate num_traits; use num_traits::cast::*; +use num_traits::Bounded; use core::{i8, i16, i32, i64, isize}; use core::{u8, u16, u32, u64, usize}; @@ -16,6 +17,7 @@ use core::{f32, f64}; #[cfg(has_i128)] use core::{i128, u128}; +use core::fmt::Debug; use core::mem; use core::num::Wrapping; @@ -317,3 +319,40 @@ fn cast_int_to_128_edge_cases() { test_edge!(usize u8 u16 u32 u64 u128); } +#[test] +fn newtype_from_primitive() { + #[derive(PartialEq, Debug)] + struct New(T); + + // minimal impl + impl FromPrimitive for New { + fn from_i64(n: i64) -> Option { + T::from_i64(n).map(New) + } + + fn from_u64(n: u64) -> Option { + T::from_u64(n).map(New) + } + } + + macro_rules! assert_eq_from { + ($( $from:ident )+) => {$( + assert_eq!(T::$from(Bounded::min_value()).map(New), + New::::$from(Bounded::min_value())); + assert_eq!(T::$from(Bounded::max_value()).map(New), + New::::$from(Bounded::max_value())); + )+} + } + + fn check() { + assert_eq_from!(from_i8 from_i16 from_i32 from_i64 from_isize); + assert_eq_from!(from_u8 from_u16 from_u32 from_u64 from_usize); + assert_eq_from!(from_f32 from_f64); + } + + macro_rules! check { + ($( $ty:ty )+) => {$( check::<$ty>(); )+} + } + check!(i8 i16 i32 i64 isize); + check!(u8 u16 u32 u64 usize); +}