From 30e8009af1233e9b6136b85f754db28f21e204ce Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Fri, 21 Feb 2020 20:42:14 +0100 Subject: [PATCH] fix Float and UFloat --- src/types/float.rs | 95 ++++++++++++++++++++++++++++++++++++--------- src/types/ufloat.rs | 51 ++++++++++++++++++++---- 2 files changed, 120 insertions(+), 26 deletions(-) diff --git a/src/types/float.rs b/src/types/float.rs index c118f69..e14b9c5 100644 --- a/src/types/float.rs +++ b/src/types/float.rs @@ -12,7 +12,7 @@ use crate::Error; /// [`NaN`]: core::f32::NAN /// [`INFINITY`]: core::f32::INFINITY /// [`NEG_INFINITY`]: core::f32::NEG_INFINITY -#[derive(Deref, Default, Debug, Copy, Clone, PartialEq, PartialOrd, Display)] +#[derive(Deref, Default, Debug, Copy, Clone, Display, PartialOrd)] pub struct Float(f32); impl Float { @@ -95,8 +95,14 @@ macro_rules! implement_from { implement_from!(i16, u16, i8, u8); +impl PartialEq for Float { + #[inline] + fn eq(&self, other: &Self) -> bool { self.0 == other.0 } +} + // convenience implementation to compare f32 with a Float. impl PartialEq for Float { + #[inline] fn eq(&self, other: &f32) -> bool { &self.0 == other } } @@ -117,11 +123,10 @@ impl Eq for Float {} impl Ord for Float { #[inline] - #[must_use] fn cmp(&self, other: &Self) -> Ordering { - if *self < *other { + if self.0 < other.0 { Ordering::Less - } else if *self == *other { + } else if self == other { Ordering::Equal } else { Ordering::Greater @@ -129,15 +134,43 @@ impl Ord for Float { } } +/// The output of Hash cannot be relied upon to be stable. The same version of +/// rust can return different values in different architectures. This is not a +/// property of the Hasher that you’re using but instead of the way Hash happens +/// to be implemented for the type you’re using (e.g., the current +/// implementation of Hash for slices of integers returns different values in +/// big and little-endian architectures). +/// +/// See #[doc(hidden)] impl ::core::hash::Hash for Float { fn hash(&self, state: &mut H) where H: ::core::hash::Hasher, { - // this should be totally fine (definitely not the most - // efficient implementation as this requires an allocation) - state.write(self.to_string().as_bytes()) + // this implementation assumes, that the internal float is: + // - not NaN + // - neither negative nor positive infinity + + // to validate those assumptions debug_assertions are here + // (those will be removed in a release build) + debug_assert!(self.0.is_finite()); + debug_assert!(!self.0.is_nan()); + + // this implementation is based on + // https://internals.rust-lang.org/t/f32-f64-should-implement-hash/5436/33 + // + // The important points are: + // - NaN == NaN (Float does not allow NaN, so this should be satisfied) + // - +0 == -0 + + if self.0 == 0.0 || self.0 == -0.0 { + state.write(&0.0_f32.to_be_bytes()); + } else { + // I do not think it matters to differentiate between architectures, that use + // big endian by default and those, that use little endian. + state.write(&self.to_be_bytes()) + } } } @@ -146,6 +179,42 @@ mod tests { use super::*; use pretty_assertions::assert_eq; + #[test] + fn test_ord() { + assert_eq!(Float::new(1.1).cmp(&Float::new(1.1)), Ordering::Equal); + assert_eq!(Float::new(1.1).cmp(&Float::new(2.1)), Ordering::Less); + assert_eq!(Float::new(1.1).cmp(&Float::new(0.1)), Ordering::Greater); + } + + #[test] + fn test_partial_ord() { + assert_eq!( + Float::new(1.1).partial_cmp(&Float::new(1.1)), + Some(Ordering::Equal) + ); + assert_eq!( + Float::new(1.1).partial_cmp(&Float::new(2.1)), + Some(Ordering::Less) + ); + assert_eq!( + Float::new(1.1).partial_cmp(&Float::new(0.1)), + Some(Ordering::Greater) + ); + } + + #[test] + fn test_eq() { + struct _AssertEq + where + Float: Eq; + } + + #[test] + fn test_partial_eq() { + assert_eq!(Float::new(1.0).eq(&Float::new(1.0)), true); + assert_eq!(Float::new(1.0).eq(&Float::new(33.3)), false); + } + #[test] fn test_display() { assert_eq!(Float::new(22.0).to_string(), "22".to_string()); @@ -185,11 +254,6 @@ mod tests { #[should_panic = "float must not be `NaN`"] fn test_new_nan() { Float::new(::core::f32::NAN); } - #[test] - fn test_partial_eq() { - assert_eq!(Float::new(1.1), 1.1); - } - #[test] fn test_as_f32() { assert_eq!(Float::new(1.1).as_f32(), 1.1_f32); @@ -212,11 +276,4 @@ mod tests { assert!(Float::try_from(::core::f32::NAN).is_err()); assert!(Float::try_from(::core::f32::NEG_INFINITY).is_err()); } - - #[test] - fn test_eq() { - struct _AssertEq - where - Float: Eq; - } } diff --git a/src/types/ufloat.rs b/src/types/ufloat.rs index a954810..4678d89 100644 --- a/src/types/ufloat.rs +++ b/src/types/ufloat.rs @@ -13,7 +13,7 @@ use crate::Error; /// [`NaN`]: core::f32::NAN /// [`INFINITY`]: core::f32::INFINITY /// [`NEG_INFINITY`]: core::f32::NEG_INFINITY -#[derive(Deref, Default, Debug, Copy, Clone, PartialEq, PartialOrd, Display)] +#[derive(Deref, Default, Debug, Copy, Clone, PartialOrd, Display)] pub struct UFloat(f32); impl UFloat { @@ -106,8 +106,16 @@ macro_rules! implement_from { implement_from!(u16, u8); +// This has to be implemented explicitly, because `Hash` is also implemented +// manually and both implementations have to agree according to clippy. +impl PartialEq for UFloat { + #[inline] + fn eq(&self, other: &Self) -> bool { self.0 == other.0 } +} + // convenience implementation to compare f32 with a Float. impl PartialEq for UFloat { + #[inline] fn eq(&self, other: &f32) -> bool { &self.0 == other } } @@ -128,11 +136,10 @@ impl Eq for UFloat {} impl Ord for UFloat { #[inline] - #[must_use] fn cmp(&self, other: &Self) -> Ordering { - if *self < *other { + if self.0 < other.0 { Ordering::Less - } else if *self == *other { + } else if self == other { Ordering::Equal } else { Ordering::Greater @@ -140,15 +147,41 @@ impl Ord for UFloat { } } +/// The output of Hash cannot be relied upon to be stable. The same version of +/// rust can return different values in different architectures. This is not a +/// property of the Hasher that you’re using but instead of the way Hash happens +/// to be implemented for the type you’re using (e.g., the current +/// implementation of Hash for slices of integers returns different values in +/// big and little-endian architectures). +/// +/// See #[doc(hidden)] impl ::core::hash::Hash for UFloat { fn hash(&self, state: &mut H) where H: ::core::hash::Hasher, { - // this should be totally fine (definitely not the most - // efficient implementation as this requires an allocation) - state.write(self.to_string().as_bytes()) + // this implementation assumes, that the internal float is: + // - positive + // - not NaN + // - neither negative nor positive infinity + + // to validate those assumptions debug_assertions are here + // (those will be removed in a release build) + debug_assert!(self.0.is_sign_positive()); + debug_assert!(self.0.is_finite()); + debug_assert!(!self.0.is_nan()); + + // this implementation is based on + // https://internals.rust-lang.org/t/f32-f64-should-implement-hash/5436/33 + // + // The important points are: + // - NaN == NaN (UFloat does not allow NaN, so this should be satisfied) + // - +0 != -0 (UFloat does not allow negative numbers, so this is fine too) + + // I do not think it matters to differentiate between architectures, that use + // big endian by default and those, that use little endian. + state.write(&self.to_be_bytes()) } } @@ -184,6 +217,10 @@ mod tests { #[should_panic = "float must be positive: `-1.1`"] fn test_new_negative() { UFloat::new(-1.1); } + #[test] + #[should_panic = "float must be positive: `0`"] + fn test_new_negative_zero() { UFloat::new(-0.0); } + #[test] #[should_panic = "float must be finite: `inf`"] fn test_new_infinite() { UFloat::new(::core::f32::INFINITY); }