Skip to content

Commit

Permalink
Changed minimum allowable Duration to use i64::MIN
Browse files Browse the repository at this point in the history
  - Changed the value of the MIN Duration to use i64::MIN instead of
    -i64::MAX. All previously-failing tests now pass. Note, the change
    for MIN.secs is for clarity and consistency only - the new and
    previous expressions here are equivalent, and result in the same
    number being computed, due to rounding. The actual difference comes
    from the change to MIN.nanos - changing the modulo to use i64::MIN
    instead of -i64::MAX results in a modulus of 192 instead of 193,
    which fixes the bug and allows the full i64 range to be used for
    representing milliseconds, aligning with the constructor behaviour.

  - Updated documentation to refer to i64::MIN instead of -i64::MAX.
  • Loading branch information
danwilliams committed Jan 26, 2024
1 parent a1989c7 commit 3417668
Showing 1 changed file with 7 additions and 21 deletions.
28 changes: 7 additions & 21 deletions src/duration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@ pub struct Duration {
nanos: i32, // Always 0 <= nanos < NANOS_PER_SEC
}

/// The minimum possible `Duration`: `-i64::MAX` milliseconds.
/// The minimum possible `Duration`: `i64::MIN` milliseconds.
pub(crate) const MIN: Duration = Duration {
secs: -i64::MAX / MILLIS_PER_SEC - 1,
nanos: NANOS_PER_SEC + (-i64::MAX % MILLIS_PER_SEC) as i32 * NANOS_PER_MILLI,
secs: i64::MIN / MILLIS_PER_SEC - 1,
nanos: NANOS_PER_SEC + (i64::MIN % MILLIS_PER_SEC) as i32 * NANOS_PER_MILLI,
};

/// The maximum possible `Duration`: `i64::MAX` milliseconds.
Expand Down Expand Up @@ -146,7 +146,7 @@ impl Duration {

/// Makes a new `Duration` with given number of seconds.
/// Panics when the duration is more than `i64::MAX` milliseconds
/// or less than `-i64::MAX` milliseconds.
/// or less than `i64::MIN` milliseconds.
#[inline]
#[must_use]
pub fn seconds(seconds: i64) -> Duration {
Expand All @@ -155,7 +155,7 @@ impl Duration {

/// Makes a new `Duration` with given number of seconds.
/// Returns `None` when the duration is more than `i64::MAX` milliseconds
/// or less than `-i64::MAX` milliseconds.
/// or less than `i64::MIN` milliseconds.
#[inline]
pub fn try_seconds(seconds: i64) -> Option<Duration> {
let d = Duration { secs: seconds, nanos: 0 };
Expand Down Expand Up @@ -305,7 +305,7 @@ impl Duration {
}
}

/// The minimum possible `Duration`: `-i64::MAX` milliseconds.
/// The minimum possible `Duration`: `i64::MIN` milliseconds.
#[inline]
pub const fn min_value() -> Duration {
MIN
Expand Down Expand Up @@ -509,7 +509,7 @@ const fn div_mod_floor_64(this: i64, other: i64) -> (i64, i64) {
#[cfg(feature = "arbitrary")]
impl arbitrary::Arbitrary<'_> for Duration {
fn arbitrary(u: &mut arbitrary::Unstructured) -> arbitrary::Result<Duration> {
const MIN_SECS: i64 = -i64::MAX / MILLIS_PER_SEC - 1;
const MIN_SECS: i64 = i64::MIN / MILLIS_PER_SEC;
const MAX_SECS: i64 = i64::MAX / MILLIS_PER_SEC;

let secs: i64 = u.int_in_range(MIN_SECS..=MAX_SECS)?;
Expand Down Expand Up @@ -860,18 +860,11 @@ mod tests {
}
#[test]
fn test_min() {
// WARNING:
// This test currently fails, because MIN is not aligned with i64::MIN which
// is used as the basis for other functions. Notably, the failure is in the
// assertion, i.e. the two values are not equal, rather than a failure to
// create a Duration with a value one millisecond less than that of MIN.
assert_eq!(
MIN.secs as i128 * 1_000_000_000 + MIN.nanos as i128,
i64::MIN as i128 * 1_000_000
);
// Ditto (fails)
assert_eq!(MIN, Duration::milliseconds(i64::MIN));
// Ditto (fails)
assert_eq!(MIN.num_milliseconds(), i64::MIN);
assert_eq!(MIN.num_microseconds(), None);
assert_eq!(MIN.num_nanoseconds(), None);
Expand Down Expand Up @@ -910,17 +903,10 @@ mod tests {
.is_none());
assert!(Duration::milliseconds(i64::MAX).checked_add(&Duration::nanoseconds(1)).is_none());

// WARNING:
// This test currently fails, because MIN is not aligned with i64::MIN which
// is used as the basis for other functions. Notably, the failure is in the
// assertion, i.e. the two values are not equal, rather than a failure to
// create a Duration with a value one millisecond less than that of MIN. The
// assertion fails here because the result of checked_sub() is None.
assert_eq!(
Duration::milliseconds(i64::MIN).checked_sub(&Duration::milliseconds(0)),
Some(Duration::milliseconds(i64::MIN))
);
// Ditto - the failure here is "`Duration - Duration` overflowed"
assert_eq!(
Duration::milliseconds(i64::MIN + 1).checked_sub(&Duration::microseconds(999)),
Some(Duration::milliseconds(i64::MIN + 2) - Duration::microseconds(1999))
Expand Down

0 comments on commit 3417668

Please sign in to comment.