From 025e8e639c5443f4c1e0089181d4bb72559022d2 Mon Sep 17 00:00:00 2001 From: Kanabenki Date: Wed, 27 Mar 2024 01:26:56 +0100 Subject: [PATCH] Fix `Ord` and `PartialOrd` differing for `FloatOrd` and optimize implementation (#12711) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective - `FloatOrd` currently has a different comparison behavior between its derived `PartialOrd` impl and manually implemented `Ord` impl (The [`Ord` doc](https://doc.rust-lang.org/std/cmp/trait.Ord.html) says this is a logic error). This might be a problem for some `std` containers/algorithms if they rely on both matching, and a footgun for Bevy users. ## Solution - Replace the `PartialEq` and `Ord` impls of `FloatOrd` with some equivalent ones producing [better assembly.](https://godbolt.org/z/jaWbjnMKx) - Manually derive `PartialOrd` with the same behavior as `Ord`, implement the comparison operators. - Add some tests. I first tried using a match-based implementation similar to the `PartialOrd` impl [of the std](https://doc.rust-lang.org/src/core/cmp.rs.html#1457) (with added NaN ordering) but I couldn't get it to produce non-branching assembly. The current implementation is based on [the one from the `ordered_float` crate](https://github.com/reem/rust-ordered-float/blob/3641f59e314030b2ffa3e6321c0d51f8ce50b385/src/lib.rs#L121), adapted since it uses a different ordering. Should this be mentionned somewhere in the code? --- ## Changelog ### Fixed - `FloatOrd` now uses the same ordering for its `PartialOrd` and `Ord` implementations. ## Migration Guide - If you were depending on the `PartialOrd` behaviour of `FloatOrd`, it has changed from matching `f32` to matching `FloatOrd`'s `Ord` ordering, never returning `None`. --- crates/bevy_utils/src/float_ord.rs | 130 ++++++++++++++++++++++++++--- 1 file changed, 117 insertions(+), 13 deletions(-) diff --git a/crates/bevy_utils/src/float_ord.rs b/crates/bevy_utils/src/float_ord.rs index 9e7931d220..2c6bbd4325 100644 --- a/crates/bevy_utils/src/float_ord.rs +++ b/crates/bevy_utils/src/float_ord.rs @@ -13,28 +13,48 @@ use std::{ /// /// Wrapping a float with `FloatOrd` breaks conformance with the standard /// by sorting `NaN` as less than all other numbers and equal to any other `NaN`. -#[derive(Debug, Copy, Clone, PartialOrd)] +#[derive(Debug, Copy, Clone)] pub struct FloatOrd(pub f32); -#[allow(clippy::derive_ord_xor_partial_ord)] +impl PartialOrd for FloatOrd { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } + + fn lt(&self, other: &Self) -> bool { + !other.le(self) + } + // If `self` is NaN, it is equal to another NaN and less than all other floats, so return true. + // If `self` isn't NaN and `other` is, the float comparison returns false, which match the `FloatOrd` ordering. + // Otherwise, a standard float comparison happens. + fn le(&self, other: &Self) -> bool { + self.0.is_nan() || self.0 <= other.0 + } + fn gt(&self, other: &Self) -> bool { + !self.le(other) + } + fn ge(&self, other: &Self) -> bool { + other.le(self) + } +} + impl Ord for FloatOrd { + #[allow(clippy::comparison_chain)] fn cmp(&self, other: &Self) -> Ordering { - self.0.partial_cmp(&other.0).unwrap_or_else(|| { - if self.0.is_nan() && !other.0.is_nan() { - Ordering::Less - } else if !self.0.is_nan() && other.0.is_nan() { - Ordering::Greater - } else { - Ordering::Equal - } - }) + if self > other { + Ordering::Greater + } else if self < other { + Ordering::Less + } else { + Ordering::Equal + } } } impl PartialEq for FloatOrd { fn eq(&self, other: &Self) -> bool { - if self.0.is_nan() && other.0.is_nan() { - true + if self.0.is_nan() { + other.0.is_nan() } else { self.0 == other.0 } @@ -64,3 +84,87 @@ impl Neg for FloatOrd { FloatOrd(-self.0) } } + +#[cfg(test)] +mod tests { + use std::hash::DefaultHasher; + + use super::*; + + const NAN: FloatOrd = FloatOrd(f32::NAN); + const ZERO: FloatOrd = FloatOrd(0.0); + const ONE: FloatOrd = FloatOrd(1.0); + + #[test] + fn float_ord_eq() { + assert_eq!(NAN, NAN); + + assert_ne!(NAN, ZERO); + assert_ne!(ZERO, NAN); + + assert_eq!(ZERO, ZERO); + } + + #[test] + fn float_ord_cmp() { + assert_eq!(NAN.cmp(&NAN), Ordering::Equal); + + assert_eq!(NAN.cmp(&ZERO), Ordering::Less); + assert_eq!(ZERO.cmp(&NAN), Ordering::Greater); + + assert_eq!(ZERO.cmp(&ZERO), Ordering::Equal); + assert_eq!(ONE.cmp(&ZERO), Ordering::Greater); + assert_eq!(ZERO.cmp(&ONE), Ordering::Less); + } + + #[test] + #[allow(clippy::nonminimal_bool)] + fn float_ord_cmp_operators() { + assert!(!(NAN < NAN)); + assert!(NAN < ZERO); + assert!(!(ZERO < NAN)); + assert!(!(ZERO < ZERO)); + assert!(ZERO < ONE); + assert!(!(ONE < ZERO)); + + assert!(!(NAN > NAN)); + assert!(!(NAN > ZERO)); + assert!(ZERO > NAN); + assert!(!(ZERO > ZERO)); + assert!(!(ZERO > ONE)); + assert!(ONE > ZERO); + + assert!(NAN <= NAN); + assert!(NAN <= ZERO); + assert!(!(ZERO <= NAN)); + assert!(ZERO <= ZERO); + assert!(ZERO <= ONE); + assert!(!(ONE <= ZERO)); + + assert!(NAN >= NAN); + assert!(!(NAN >= ZERO)); + assert!(ZERO >= NAN); + assert!(ZERO >= ZERO); + assert!(!(ZERO >= ONE)); + assert!(ONE >= ZERO); + } + + #[test] + fn float_ord_hash() { + let hash = |num| { + let mut h = DefaultHasher::new(); + FloatOrd(num).hash(&mut h); + h.finish() + }; + + assert_ne!((-0.0f32).to_bits(), 0.0f32.to_bits()); + assert_eq!(hash(-0.0), hash(0.0)); + + let nan_1 = f32::from_bits(0b0111_1111_1000_0000_0000_0000_0000_0001); + assert!(nan_1.is_nan()); + let nan_2 = f32::from_bits(0b0111_1111_1000_0000_0000_0000_0000_0010); + assert!(nan_2.is_nan()); + assert_ne!(nan_1.to_bits(), nan_2.to_bits()); + assert_eq!(hash(nan_1), hash(nan_2)); + } +}