Skip to content

Commit

Permalink
Fix panic when formatting TimeStamp::MIN (#838)
Browse files Browse the repository at this point in the history
In order to print the minus sign of times, we detect the need for the
minus sign, print it and then print the absolute value of the time.
However, integers have a minimum value of which the absolute value is
larger than the maximum value of the integer. So for example `i8` has a
minimum value of `-128` and a maximum value of `127`. The absolute value
therefore can't be represented. This caused a panic when formatting
`TimeStamp::MIN` when overflow checks are enabled, such as when
compiling with the debug profile.

There was also a big lack of tests for the formatting of times. Tests
are now included for all the important cases.
  • Loading branch information
CryZe committed Aug 28, 2024
1 parent eccd5da commit b0ff646
Show file tree
Hide file tree
Showing 6 changed files with 760 additions and 26 deletions.
112 changes: 111 additions & 1 deletion src/timing/formatter/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl Display for Inner {
// Since, this Formatter is used for writing out split files, we
// have to use an ASCII Minus here.
f.write_str(ASCII_MINUS)?;
((-total_seconds) as u64, (-nanoseconds) as u32)
(total_seconds.wrapping_neg() as u64, (-nanoseconds) as u32)
} else {
(total_seconds as u64, nanoseconds as u32)
};
Expand Down Expand Up @@ -86,3 +86,113 @@ impl Display for Inner {
}
}
}

#[cfg(test)]
mod tests {
use core::str::FromStr;

use super::*;

#[test]
fn min() {
// This verifies that flipping the sign of the minimum value doesn't
// cause a panic.
let time = TimeSpan::from(crate::platform::Duration::MIN);
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "-106751991167300.15:30:08.999999999");
}

#[test]
fn max() {
let time = TimeSpan::from(crate::platform::Duration::MAX);
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "106751991167300.15:30:07.999999999");
}

#[test]
fn zero() {
let time = TimeSpan::zero();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "00:00:00.000000000");
}

#[test]
fn empty() {
let inner = Complete.format(None);
assert_eq!(inner.to_string(), "00:00:00.000000000");
}

#[test]
fn slightly_positive() {
let time = TimeSpan::from_str("0.000000001").unwrap();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "00:00:00.000000001");

assert_eq!(
Complete.format(TimeSpan::from_seconds(0.5)).to_string(),
"00:00:00.500000000"
);
assert_eq!(
Complete.format(TimeSpan::from_seconds(1.5)).to_string(),
"00:00:01.500000000"
);
}

#[test]
fn slightly_negative() {
let time = TimeSpan::from_str("-0.000000001").unwrap();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "-00:00:00.000000001");

assert_eq!(
Complete.format(TimeSpan::from_seconds(-1.5)).to_string(),
"-00:00:01.500000000"
);
assert_eq!(
Complete.format(TimeSpan::from_seconds(-0.5)).to_string(),
"-00:00:00.500000000"
);
}

#[test]
fn seconds() {
let time = TimeSpan::from_str("23.1234").unwrap();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "00:00:23.123400000");
}

#[test]
fn minutes() {
let time = TimeSpan::from_str("12:34.987654321").unwrap();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "00:12:34.987654321");
}

#[test]
fn hours() {
let time = TimeSpan::from_str("12:34:56.123456789").unwrap();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "12:34:56.123456789");
}

#[test]
fn negative() {
let time = TimeSpan::from_str("-12:34:56.123456789").unwrap();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "-12:34:56.123456789");
}

#[test]
fn days() {
let time = TimeSpan::from_str("2148:34:56.123456789").unwrap();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "89.12:34:56.123456789");
}

#[test]
fn negative_days() {
let time = TimeSpan::from_str("-2148:34:56.123456789").unwrap();
let inner = Complete.format(Some(time));
assert_eq!(inner.to_string(), "-89.12:34:56.123456789");
}
}
106 changes: 105 additions & 1 deletion src/timing/formatter/days.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl Display for Inner {
let (total_seconds, nanoseconds) = time.to_seconds_and_subsec_nanoseconds();
let total_seconds = if (total_seconds | nanoseconds as i64) < 0 {
f.write_str(MINUS)?;
(-total_seconds) as u64
total_seconds.wrapping_neg() as u64
} else {
total_seconds as u64
};
Expand Down Expand Up @@ -82,3 +82,107 @@ impl Display for Inner {
}
}
}

#[cfg(test)]
mod tests {
use core::str::FromStr;

use super::*;

#[test]
fn min() {
// This verifies that flipping the sign of the minimum value doesn't
// cause a panic.
let time = TimeSpan::from(crate::platform::Duration::MIN);
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "−106751991167300d 15:30:08");
}

#[test]
fn max() {
let time = TimeSpan::from(crate::platform::Duration::MAX);
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "106751991167300d 15:30:07");
}

#[test]
fn zero() {
let time = TimeSpan::zero();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "0:00");
}

#[test]
fn empty() {
let inner = Days.format(None);
assert_eq!(inner.to_string(), "0:00");
}

#[test]
fn slightly_positive() {
let time = TimeSpan::from_str("0.000000001").unwrap();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "0:00");

assert_eq!(Days.format(TimeSpan::from_seconds(0.5)).to_string(), "0:00");
assert_eq!(Days.format(TimeSpan::from_seconds(1.5)).to_string(), "0:01");
}

#[test]
fn slightly_negative() {
let time = TimeSpan::from_str("-0.000000001").unwrap();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "−0:00");

assert_eq!(
Days.format(TimeSpan::from_seconds(-1.5)).to_string(),
"−0:01"
);
assert_eq!(
Days.format(TimeSpan::from_seconds(-0.5)).to_string(),
"−0:00"
);
}

#[test]
fn seconds() {
let time = TimeSpan::from_str("23.1234").unwrap();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "0:23");
}

#[test]
fn minutes() {
let time = TimeSpan::from_str("12:34.987654321").unwrap();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "12:34");
}

#[test]
fn hours() {
let time = TimeSpan::from_str("12:34:56.123456789").unwrap();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "12:34:56");
}

#[test]
fn negative() {
let time = TimeSpan::from_str("-12:34:56.123456789").unwrap();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "−12:34:56");
}

#[test]
fn days() {
let time = TimeSpan::from_str("2148:34:56.123456789").unwrap();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "89d 12:34:56");
}

#[test]
fn negative_days() {
let time = TimeSpan::from_str("-2148:34:56.123456789").unwrap();
let inner = Days.format(Some(time));
assert_eq!(inner.to_string(), "−89d 12:34:56");
}
}
125 changes: 113 additions & 12 deletions src/timing/formatter/delta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ impl Delta {
pub const fn with_decimal_dropping() -> Self {
Delta(true, Accuracy::Tenths)
}

/// Creates a new Delta Time Formatter that does not drop the fractional
/// part and uses tenths when showing the fractional part.
pub const fn without_decimal_dropping() -> Self {
Delta(false, Accuracy::Tenths)
}
}

impl Default for Delta {
Expand Down Expand Up @@ -78,7 +84,7 @@ impl Display for Inner {
let bit_or = total_seconds | nanoseconds as i64;
let (total_seconds, nanoseconds) = if bit_or < 0 {
f.write_str(MINUS)?;
((-total_seconds) as u64, (-nanoseconds) as u32)
(total_seconds.wrapping_neg() as u64, (-nanoseconds) as u32)
} else {
if bit_or > 0 {
f.write_str(PLUS)?;
Expand Down Expand Up @@ -122,10 +128,61 @@ impl Display for Inner {

#[cfg(test)]
mod tests {
use core::str::FromStr;

use super::*;

#[test]
fn sign_works() {
fn min() {
// This verifies that flipping the sign of the minimum value doesn't
// cause a panic.
let time = TimeSpan::from(crate::platform::Duration::MIN);
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "−2562047788015215:30:08");
}

#[test]
fn max() {
let time = TimeSpan::from(crate::platform::Duration::MAX);
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "+2562047788015215:30:07");
}

#[test]
fn zero() {
let time = TimeSpan::zero();
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "0.0");
}

#[test]
fn empty() {
let inner = Delta::new().format(None);
assert_eq!(inner.to_string(), "—");
}

#[test]
fn slightly_positive() {
let time = TimeSpan::from_str("0.000000001").unwrap();
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "+0.0");

assert_eq!(
Delta::new().format(TimeSpan::from_seconds(0.5)).to_string(),
"+0.5"
);
assert_eq!(
Delta::new().format(TimeSpan::from_seconds(1.5)).to_string(),
"+1.5"
);
}

#[test]
fn slightly_negative() {
let time = TimeSpan::from_str("-0.000000001").unwrap();
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "−0.0");

assert_eq!(
Delta::new()
.format(TimeSpan::from_seconds(-1.5))
Expand All @@ -138,17 +195,61 @@ mod tests {
.to_string(),
"−0.5"
);
}

// We drop the sign entirely when it's exactly 0.
assert_eq!(Delta::new().format(TimeSpan::zero()).to_string(), "0.0");
#[test]
fn seconds() {
let time = TimeSpan::from_str("23.1234").unwrap();
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "+23.1");
}

assert_eq!(
Delta::new().format(TimeSpan::from_seconds(0.5)).to_string(),
"+0.5"
);
assert_eq!(
Delta::new().format(TimeSpan::from_seconds(1.5)).to_string(),
"+1.5"
);
#[test]
fn minutes_with_decimal_dropping() {
let time = TimeSpan::from_str("12:34.987654321").unwrap();
let inner = Delta::with_decimal_dropping().format(Some(time));
assert_eq!(inner.to_string(), "+12:34");
}

#[test]
fn minutes_without_decimal_dropping() {
let time = TimeSpan::from_str("12:34.987654321").unwrap();
let inner = Delta::without_decimal_dropping().format(Some(time));
assert_eq!(inner.to_string(), "+12:34.9");
}

#[test]
fn hours_with_decimal_dropping() {
let time = TimeSpan::from_str("12:34:56.123456789").unwrap();
let inner = Delta::with_decimal_dropping().format(Some(time));
assert_eq!(inner.to_string(), "+12:34:56");
}

#[test]
fn hours_without_decimal_dropping() {
let time = TimeSpan::from_str("12:34:56.123456789").unwrap();
let inner = Delta::without_decimal_dropping().format(Some(time));
assert_eq!(inner.to_string(), "+12:34:56.1");
}

#[test]
fn negative() {
let time = TimeSpan::from_str("-12:34:56.123456789").unwrap();
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "−12:34:56");
}

#[test]
fn days() {
let time = TimeSpan::from_str("2148:34:56.123456789").unwrap();
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "+2148:34:56");
}

#[test]
fn negative_days() {
let time = TimeSpan::from_str("-2148:34:56.123456789").unwrap();
let inner = Delta::new().format(Some(time));
assert_eq!(inner.to_string(), "−2148:34:56");
}
}
Loading

0 comments on commit b0ff646

Please sign in to comment.