Skip to content

Commit

Permalink
Fix implementation of Duration::new
Browse files Browse the repository at this point in the history
A couple hacks are needed to maintain it being `const fn`. Generally
speaking, this patch just ensures that the sign of the nanoseconds
component matches the sign of the seconds component. To do this, it is
necessary to carry over any remainder.
  • Loading branch information
jhpratt committed Sep 26, 2020
1 parent b398aaf commit ad4740f
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 8 deletions.
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ Versioning].

---

## 0.2.22 [2020-09-25]

### Fixed

- Solaris & Illumos now successfully build.
- `Duration::new` could previously result in an inconsistent internal state.
This led to some odd situations where a `Duration` could be both positive and
negative. This has been fixed such that the internal state maintains its
invariants.

## 0.2.21 [2020-09-20]

### Changed
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "time"
version = "0.2.21"
version = "0.2.22"
authors = ["Jacob Pratt <the.z.cuber@gmail.com>"]
edition = "2018"
repository = "https://github.com/time-rs/time"
Expand Down
66 changes: 63 additions & 3 deletions src/duration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ impl Duration {
}

/// Create a new `Duration` with the provided seconds and nanoseconds. If
/// nanoseconds is at least 10<sup>9</sup>, it will wrap to the number of
/// nanoseconds is at least ±10<sup>9</sup>, it will wrap to the number of
/// seconds.
///
/// ```rust
Expand All @@ -257,13 +257,73 @@ impl Duration {
/// assert_eq!(Duration::new(-1, 0), (-1).seconds());
/// assert_eq!(Duration::new(1, 2_000_000_000), 3.seconds());
/// ```
// FIXME This code is stupidly complex for the sole reason of maintaining
// back-compatibility with Rust 1.32.0. The equivalent code is commented out
// immediately below this function implementation. Thankfully, the compiler
// is able to do quite well at deduplicating the operations.
pub const fn new(seconds: i64, nanoseconds: i32) -> Self {
Self {
seconds: seconds + nanoseconds as i64 / 1_000_000_000,
nanoseconds: nanoseconds % 1_000_000_000,
seconds: (seconds + nanoseconds as i64 / 1_000_000_000)
+ (((((seconds + nanoseconds as i64 / 1_000_000_000) > 0) as i8
- ((seconds + nanoseconds as i64 / 1_000_000_000) < 0) as i8)
== -1)
& ((((nanoseconds % 1_000_000_000) > 0) as i8
- ((nanoseconds % 1_000_000_000) < 0) as i8)
== 1)) as i64
- (((((seconds + nanoseconds as i64 / 1_000_000_000) > 0) as i8
- ((seconds + nanoseconds as i64 / 1_000_000_000) < 0) as i8)
== 1)
& ((((nanoseconds % 1_000_000_000) > 0) as i8
- ((nanoseconds % 1_000_000_000) < 0) as i8)
== -1)) as i64,
nanoseconds: (nanoseconds % 1_000_000_000)
+ 1_000_000_000
* ((((((seconds + nanoseconds as i64 / 1_000_000_000) > 0) as i8
- ((seconds + nanoseconds as i64 / 1_000_000_000) < 0) as i8)
== 1)
& ((((nanoseconds % 1_000_000_000) > 0) as i8
- ((nanoseconds % 1_000_000_000) < 0) as i8)
== -1)) as i32
- (((((seconds + nanoseconds as i64 / 1_000_000_000) > 0) as i8
- ((seconds + nanoseconds as i64 / 1_000_000_000) < 0) as i8)
== -1)
& ((((nanoseconds % 1_000_000_000) > 0) as i8
- ((nanoseconds % 1_000_000_000) < 0) as i8)
== 1)) as i32),
}
}

// pub const fn new(mut seconds: i64, mut nanoseconds: i32) -> Self {
// seconds += nanoseconds as i64 / 1_000_000_000;
// nanoseconds %= 1_000_000_000;
//
// // Alternatively, we can use `(nano)seconds.signum()` once it is
// // stabilized within `const fn`. The behavior is identical.
// let seconds_sign = match seconds {
// n if n > 0 => 1,
// 0 => 0,
// _ => -1,
// };
// let nanoseconds_sign = match nanoseconds {
// n if n > 0 => 1,
// 0 => 0,
// _ => -1,
// };
//
// if seconds_sign == 1 && nanoseconds_sign == -1 {
// seconds -= 1;
// nanoseconds += 1_000_000_000;
// } else if seconds_sign == -1 && nanoseconds_sign == 1 {
// seconds += 1;
// nanoseconds -= 1_000_000_000;
// }
//
// Self {
// seconds,
// nanoseconds,
// }
// }

/// Create a new `Duration` with the given number of weeks. Equivalent to
/// `Duration::seconds(weeks * 604_800)`.
///
Expand Down
22 changes: 18 additions & 4 deletions tests/duration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,24 @@ fn new() {
assert_eq!(Duration::new(-1, 0), (-1).seconds());
assert_eq!(Duration::new(1, 2_000_000_000), 3.seconds());

assert!(Duration::new(0, 0).is_zero());
assert!(Duration::new(0, 1_000_000_000).is_positive());
assert!(Duration::new(-1, 1_000_000_000).is_zero());
assert!(Duration::new(-2, 1_000_000_000).is_negative());
assert_eq!(Duration::new(0, 0), 0.seconds());
assert_eq!(Duration::new(0, 1_000_000_000), 1.seconds());
assert_eq!(Duration::new(-1, 1_000_000_000), 0.seconds());
assert_eq!(Duration::new(-2, 1_000_000_000), (-1).seconds());

assert_eq!(Duration::new(1, -1), 999_999_999.nanoseconds());
assert_eq!(Duration::new(-1, 1), (-999_999_999).nanoseconds());
assert_eq!(Duration::new(1, 1), 1_000_000_001.nanoseconds());
assert_eq!(Duration::new(-1, -1), (-1_000_000_001).nanoseconds());
assert_eq!(Duration::new(0, 1), 1.nanoseconds());
assert_eq!(Duration::new(0, -1), (-1).nanoseconds());

assert_eq!(Duration::new(-1, 1_400_000_000), 400.milliseconds());
assert_eq!(Duration::new(-2, 1_400_000_000), (-600).milliseconds());
assert_eq!(Duration::new(-3, 1_400_000_000), (-1_600).milliseconds());
assert_eq!(Duration::new(1, -1_400_000_000), (-400).milliseconds());
assert_eq!(Duration::new(2, -1_400_000_000), 600.milliseconds());
assert_eq!(Duration::new(3, -1_400_000_000), 1_600.milliseconds());
}

#[test]
Expand Down

0 comments on commit ad4740f

Please sign in to comment.