Skip to content

Commit

Permalink
timestamp: make saturating arithmetic panic for non-uniform units
Browse files Browse the repository at this point in the history
The saturating methods *should* return an error. But I goofed on the
API. So at this point, our two choices are: leave it as-is, which
produces completely incorrect results when there are non-zero units of
days or greater, or panic when it occurs. I think a panic is better.

In Jiff 0.2, these APIs will be made fallible. And hopefully by then
we'll have truly infallible APIs that accept an "absolute" duration.

Partially addresses #36
  • Loading branch information
BurntSushi committed Jul 26, 2024
1 parent 7f1d3c3 commit c34da2b
Showing 1 changed file with 32 additions and 0 deletions.
32 changes: 32 additions & 0 deletions src/timestamp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1386,6 +1386,13 @@ impl Timestamp {
/// Add the given span of time to this timestamp. If the sum would overflow
/// the minimum or maximum timestamp values, then the result saturates.
///
/// # Panics
///
/// This panics if the given `Span` contains any non-zero units greater
/// than hours. In `jiff 0.2`, this panic will be changed to an error. It
/// panics in `jiff 0.1` to avoid giving incorrect results. (It was an
/// oversight to make this method infallible.)
///
/// # Example
///
/// This example shows that arithmetic saturates on overflow.
Expand All @@ -1404,6 +1411,12 @@ impl Timestamp {
/// ```
#[inline]
pub fn saturating_add(self, span: Span) -> Timestamp {
if let Some(err) = span.smallest_non_time_non_zero_unit_error() {
panic!(
"saturing Timestamp arithmetic \
requires only time units: {err}"
)
}
self.checked_add(span).unwrap_or_else(|_| {
if span.is_negative() {
Timestamp::MIN
Expand All @@ -1417,6 +1430,13 @@ impl Timestamp {
/// would overflow the minimum or maximum timestamp values, then the result
/// saturates.
///
/// # Panics
///
/// This panics if the given `Span` contains any non-zero units greater
/// than hours. In `jiff 0.2`, this panic will be changed to an error. It
/// panics in `jiff 0.1` to avoid giving incorrect results. (It was an
/// oversight to make this method infallible.)
///
/// # Example
///
/// This example shows that arithmetic saturates on overflow.
Expand Down Expand Up @@ -3087,6 +3107,18 @@ mod tests {
assert_eq!(inst, got);
}

#[test]
#[should_panic]
fn timestamp_saturating_add() {
Timestamp::MIN.saturating_add(Span::new().days(1));
}

#[test]
#[should_panic]
fn timestamp_saturating_sub() {
Timestamp::MAX.saturating_sub(Span::new().days(1));
}

quickcheck::quickcheck! {
fn prop_unix_seconds_roundtrip(t: Timestamp) -> quickcheck::TestResult {
let dt = t.to_zoned(TimeZone::UTC).datetime();
Expand Down

0 comments on commit c34da2b

Please sign in to comment.