From ae9d97dfca9ba7816db88f95e0a2d1cfcd4524fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Laignel?= Date: Tue, 13 Oct 2020 17:17:46 +0200 Subject: [PATCH] ClockTime & opt Formats: fix PartialOrd impl, remove Ord and add min & max - `PartialOrd` was returning `true` for expressions such as - `ClockTime::none() < ClockTime::from_seconds(1)`. - `ClockTime::from_seconds(1) > ClockTime::none()`. - Remove `Ord` because `ClockTime` is not a total order due to `ClockTime::none()`. See test `not_ord`. This also applies to others `Format(Option<{u32,u64}>)` types. --- gstreamer/src/clock_time.rs | 218 ++++++++++++++++++++++++++++++------ gstreamer/src/format.rs | 19 +++- gstreamer/src/lib.rs | 1 + 3 files changed, 199 insertions(+), 39 deletions(-) diff --git a/gstreamer/src/clock_time.rs b/gstreamer/src/clock_time.rs index e9fe463a8..a88015da7 100644 --- a/gstreamer/src/clock_time.rs +++ b/gstreamer/src/clock_time.rs @@ -12,7 +12,7 @@ use gst_sys; use std::time::Duration; use std::{cmp, convert, fmt}; -#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, Debug, Default)] +#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug, Default)] pub struct ClockTime(pub Option); impl ClockTime { @@ -63,44 +63,81 @@ impl ClockTime { skip_assert_initialized!(); nseconds * ::NSECOND } - - pub fn zero() -> ClockTime { - ClockTime(Some(0)) - } - - // FIXME `matches!` was introduced in rustc 1.42.0, current MSRV is 1.41.0 - #[allow(clippy::match_like_matches_macro)] - pub fn is_zero(&self) -> bool { - match self.0 { - Some(0) => true, - _ => false, - } - } - - pub fn none() -> ClockTime { - ClockTime(None) - } } -impl ClockTime { - #[must_use = "this returns the result of the operation, without modifying the original"] - #[inline] - pub fn saturating_add(self, rhs: ClockTime) -> Option { - match (self.0, rhs.0) { - (Some(this), Some(rhs)) => Some(ClockTime(Some(this.saturating_add(rhs)))), - _ => None, - } - } +// This macro is also used by formats with an inner Option. +// It is defined here because the format module depends on ClockTime. +macro_rules! impl_common_ops_for_opt_int( + ($name:ident) => { + impl $name { + #[must_use = "this returns the result of the operation, without modifying the original"] + #[inline] + pub fn saturating_add(self, rhs: Self) -> Option { + match (self.0, rhs.0) { + (Some(this), Some(rhs)) => Some(Self(Some(this.saturating_add(rhs)))), + _ => None, + } + } - #[must_use = "this returns the result of the operation, without modifying the original"] - #[inline] - pub fn saturating_sub(self, rhs: ClockTime) -> Option { - match (self.0, rhs.0) { - (Some(this), Some(rhs)) => Some(ClockTime(Some(this.saturating_sub(rhs)))), - _ => None, + #[must_use = "this returns the result of the operation, without modifying the original"] + #[inline] + pub fn saturating_sub(self, rhs: Self) -> Option { + match (self.0, rhs.0) { + (Some(this), Some(rhs)) => Some(Self(Some(this.saturating_sub(rhs)))), + _ => None, + } + } + + #[must_use = "this returns the result of the operation, without modifying the original"] + #[inline] + pub fn min(self, rhs: Self) -> Option { + match (self.0, rhs.0) { + (Some(this), Some(rhs)) => Some(Self(Some(this.min(rhs)))), + _ => None, + } + } + + #[must_use = "this returns the result of the operation, without modifying the original"] + #[inline] + pub fn max(self, rhs: Self) -> Option { + match (self.0, rhs.0) { + (Some(this), Some(rhs)) => Some(Self(Some(this.max(rhs)))), + _ => None, + } + } + + pub fn zero() -> Self { + Self(Some(0)) + } + + // FIXME `matches!` was introduced in rustc 1.42.0, current MSRV is 1.41.0 + // FIXME uncomment when CI can upgrade to 1.47.1 + //#[allow(clippy::match_like_matches_macro)] + pub fn is_zero(&self) -> bool { + match self.0 { + Some(0) => true, + _ => false, + } + } + + pub fn none() -> Self { + Self(None) + } } - } -} + + impl cmp::PartialOrd for $name { + fn partial_cmp(&self, other: &Self) -> Option { + match (self.0, other.0) { + (Some(this), Some(other)) => this.partial_cmp(&other), + (None, None) => Some(cmp::Ordering::Equal), + _ => None, + } + } + } + }; +); + +impl_common_ops_for_opt_int!(ClockTime); impl fmt::Display for ClockTime { #[allow(clippy::many_single_char_names)] @@ -262,4 +299,115 @@ mod tests { assert!(ct_1.saturating_sub(ct_none).is_none()); assert!(ct_none.saturating_sub(ct_1).is_none()); } + + #[test] + #[allow(clippy::eq_op)] + fn eq() { + let ct_10 = ClockTime::from_mseconds(10); + let ct_10_2 = ClockTime::from_mseconds(10); + let ct_10_3 = ClockTime::from_mseconds(10); + let ct_20 = ClockTime::from_mseconds(20); + + let ct_none = ClockTime::none(); + let ct_none_2 = ClockTime::none(); + let ct_none_3 = ClockTime::none(); + + // ## Eq + + // ### (a == b) and (a != b) are strict inverses + assert!(ct_10 == ct_10_2); + assert_ne!(ct_10 == ct_10_2, ct_10 != ct_10_2); + + assert!(ct_10 != ct_20); + assert_ne!(ct_10 == ct_20, ct_10 != ct_20); + + assert!(ct_none == ct_none_2); + assert_ne!(ct_none == ct_none_2, ct_none != ct_none_2); + + assert!(ct_10 != ct_none); + assert_ne!(ct_10 == ct_none, ct_10 != ct_none); + + assert!(ct_none != ct_10); + assert_ne!(ct_none == ct_10, ct_none != ct_10); + + // ### Reflexivity (a == a) + assert!(ct_10 == ct_10); + assert!(ct_none == ct_none); + + // ## PartialEq + + // ### Symmetric (a == b) => (b == a) + assert!((ct_10 == ct_10_2) && (ct_10_2 == ct_10)); + assert!((ct_none == ct_none_2) && (ct_none_2 == ct_none)); + + // ### Transitive (a == b) and (b == c) => (a == c) + assert!((ct_10 == ct_10_2) && (ct_10_2 == ct_10_3) && (ct_10 == ct_10_3)); + assert!((ct_none == ct_none_2) && (ct_none_2 == ct_none_3) && (ct_none == ct_none_3)); + } + + #[test] + #[allow(clippy::neg_cmp_op_on_partial_ord)] + fn partial_ord() { + let ct_10 = ClockTime::from_mseconds(10); + let ct_20 = ClockTime::from_mseconds(20); + let ct_30 = ClockTime::from_mseconds(30); + + let ct_none = ClockTime::none(); + + // Special cases + assert_eq!(ct_10 < ct_none, false); + assert_eq!(ct_10 > ct_none, false); + assert_eq!(ct_none < ct_10, false); + assert_eq!(ct_none > ct_10, false); + + // Asymmetric a < b => !(a > b) + // a < b => !(a > b) + assert!((ct_10 < ct_20) && !(ct_10 > ct_20)); + // a > b => !(a < b) + assert!((ct_20 > ct_10) && !(ct_20 < ct_10)); + + // Transitive + // a < b and b < c => a < c + assert!((ct_10 < ct_20) && (ct_20 < ct_30) && (ct_10 < ct_30)); + // a > b and b > c => a > c + assert!((ct_30 > ct_20) && (ct_20 > ct_10) && (ct_30 > ct_10)); + } + + #[test] + fn not_ord() { + let ct_10 = ClockTime::from_mseconds(10); + let ct_20 = ClockTime::from_mseconds(20); + let ct_none = ClockTime::none(); + + // Total & Antisymmetric exactly one of a < b, a == b or a > b is true + + assert!((ct_10 < ct_20) ^ (ct_10 == ct_20) ^ (ct_10 > ct_20)); + + // Not Ord due to: + assert_eq!( + (ct_10 < ct_none) ^ (ct_10 == ct_none) ^ (ct_10 > ct_none), + false + ); + assert_eq!( + (ct_none < ct_10) ^ (ct_none == ct_10) ^ (ct_none > ct_10), + false + ); + } + + #[test] + fn min_max() { + let ct_10 = ClockTime::from_nseconds(10); + let ct_20 = ClockTime::from_nseconds(20); + let ct_none = ClockTime::none(); + + assert_eq!(ct_10.min(ct_20).unwrap(), ct_10); + assert_eq!(ct_20.min(ct_10).unwrap(), ct_10); + assert!(ct_none.min(ct_10).is_none()); + assert!(ct_20.min(ct_none).is_none()); + + assert_eq!(ct_10.max(ct_20).unwrap(), ct_20); + assert_eq!(ct_20.max(ct_10).unwrap(), ct_20); + assert!(ct_none.max(ct_10).is_none()); + assert!(ct_20.max(ct_none).is_none()); + } } diff --git a/gstreamer/src/format.rs b/gstreamer/src/format.rs index 92b353381..cea74a9ab 100644 --- a/gstreamer/src/format.rs +++ b/gstreamer/src/format.rs @@ -13,6 +13,8 @@ use thiserror::Error; use ClockTime; use Format; +use std::cmp; + #[derive(PartialEq, Eq, Hash, Clone, Copy, Debug)] #[cfg_attr(feature = "ser_de", derive(Serialize, Deserialize))] pub enum GenericFormattedValue { @@ -27,15 +29,24 @@ pub enum GenericFormattedValue { #[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, Debug, Default)] pub struct Undefined(pub i64); -#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, Debug, Default)] + +#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug, Default)] pub struct Default(pub Option); -#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, Debug, Default)] +impl_common_ops_for_opt_int!(Default); + +#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug, Default)] pub struct Bytes(pub Option); +impl_common_ops_for_opt_int!(Bytes); + pub type Time = ClockTime; -#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, Debug, Default)] + +#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug, Default)] pub struct Buffers(pub Option); -#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, Debug, Default)] +impl_common_ops_for_opt_int!(Buffers); + +#[derive(PartialEq, Eq, Hash, Clone, Copy, Debug, Default)] pub struct Percent(pub Option); +impl_common_ops_for_opt_int!(Percent); #[derive(Clone, Copy, Debug, PartialEq, Eq, Error)] #[error("invalid generic value format")] diff --git a/gstreamer/src/lib.rs b/gstreamer/src/lib.rs index 235af8e64..1230edbe4 100644 --- a/gstreamer/src/lib.rs +++ b/gstreamer/src/lib.rs @@ -188,6 +188,7 @@ cfg_if! { } mod child_proxy; +#[macro_use] mod clock_time; #[cfg(feature = "ser_de")] mod clock_time_serde;