Skip to content

/rust

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libstd: Refactor Duration. #16626

Merged
merged 3 commits into from Aug 28, 2014

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ruuda @lilyball @bors @brson
@ruuda
Copy link
Contributor

ruuda commented Aug 20, 2014

This changes the internal representation of Duration from

days: i32,
secs: i32,
nanos: u32

to

secs: i64,
nanos: i32

This resolves #16466. Note that nanos is an i32 and not u32 as suggested, because i32 is easier to deal with, and it is not exposed anyway. Some methods now take i64 instead of i32 due to the increased range. Some methods, like num_milliseconds, now return an Option<i64> instead of i64, because the range of Duration is now larger than e.g. 2^63 milliseconds.

A few remarks:

  • Negating MIN is impossible. I chose to return MAX as -MIN, but it is one nanosecond less than the actual negation. Is this the desired behaviour?
  • In std::io::timer, some functions accept a Duration, which is internally converted into a number of milliseconds. However, the range of Duration is now larger than 2^64 milliseconds. There is already a FIXME in the file that this should be addressed (without a ticket number though). I chose to silently use 0 ms if the duration is too long. Is that right, as long as the backend still uses milliseconds?
  • Negative durations are not formatted correctly, but they were not formatted correctly before either.
@ruuda
libstd: Refactor Duration. 
This changes the internal representation of `Duration` from

    days: i32,
    secs: i32,
    nanos: u32

to

    secs: i64,
    nanos: i32

This resolves #16466. Some methods now take `i64` instead of `i32` due
to the increased range. Some methods, like `num_milliseconds`, now
return an `Option<i64>` instead of `i64`, because the range of
`Duration` is now larger than e.g. 2^63 milliseconds.
Loading status checks…
39133ef
@lilyball

This comment has been minimized.

Sign in to view
Copy link
Contributor

lilyball commented Aug 20, 2014

2^63 milliseconds is just shy of 300 million years. Do we really need to support the full range that i64 seconds gives us? It might be a better API to just restrict Duration to 2^63 milliseconds, which leaves num_milliseconds as returning an i64 and fixes the min/max issue.
@ruuda

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

ruuda commented Aug 21, 2014

We could restrict it to i64 milliseconds, but std::io::timer would still need to handle the case where the duration is negative.
@lilyball

This comment has been minimized.

Sign in to view
Copy link
Contributor

lilyball commented Aug 21, 2014

As I said on the previous PR, I think it's acceptable for std::io::timer to treat negative durations the same as 0. After all, it only makes a best attempt to meet the requested duration, and it can't get any better than 0 when given a negative duration.
@ruuda
libstd: Limit Duration range to i64 milliseconds. 
This enables `num_milliseconds` to return an `i64` again instead of
`Option<i64>`, because it is guaranteed not to overflow.

The Duration range is now rougly 300e6 years (positive and negative),
whereas it was 300e9 years previously. To put these numbers in
perspective, 300e9 years is about 21 times the age of the universe
(according to Wolfram|Alpha). 300e6 years is about 1/15 of the age of
the earth (according to Wolfram|Alpha).
Loading status checks…
26af5da

@ruuda ruuda referenced this pull request Aug 21, 2014

Merged

libtime: Use Duration in Timespec arithmetic. #16650

@brson

This comment has been minimized.

Sign in to view
Copy link

brson commented on 26af5da Aug 26, 2014

r+ Thanks!
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented on 26af5da Aug 26, 2014

saw approval from brson
at ruuda@26af5da

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Aug 26, 2014

merging ruud-v-a/rust/duration-reform = 26af5da into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Aug 26, 2014

ruud-v-a/rust/duration-reform = 26af5da merged ok, testing candidate = 5bd4517

bors added a commit that referenced this pull request Aug 26, 2014

@bors
auto merge of #16626 : ruud-v-a/rust/duration-reform, r=brson 
This changes the internal representation of `Duration` from

    days: i32,
    secs: i32,
    nanos: u32

to

    secs: i64,
    nanos: i32

This resolves #16466. Note that `nanos` is an `i32` and not `u32` as suggested, because `i32` is easier to deal with, and it is not exposed anyway. Some methods now take `i64` instead of `i32` due to the increased range. Some methods, like `num_milliseconds`, now return an `Option<i64>` instead of `i64`, because the range of `Duration` is now larger than e.g. 2^63 milliseconds.

A few remarks:
- Negating `MIN` is impossible. I chose to return `MAX` as `-MIN`, but it is one nanosecond less than the actual negation. Is this the desired behaviour?
- In `std::io::timer`, some functions accept a `Duration`, which is internally converted into a number of milliseconds. However, the range of `Duration` is now larger than 2^64 milliseconds. There is already a FIXME in the file that this should be addressed (without a ticket number though). I chose to silently use 0 ms if the duration is too long. Is that right, as long as the backend still uses milliseconds?
- Negative durations are not formatted correctly, but they were not formatted correctly before either.
5bd4517
@ruuda

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

ruuda commented Aug 28, 2014

The failures don’t seem to be related to the changes ... is there something I should fix?
src/libstd/time/duration.rs Outdated
assert!(Duration::days(MAX_DAYS).checked_add(&Duration::seconds(86400)).is_none());
assert_eq!(Duration::milliseconds(i64::MAX - 1).checked_add(&Duration::microseconds(999)),
Some(Duration::milliseconds(i64::MAX - 2) + Duration::microseconds(1999)));
assert!(Duration::milliseconds(i64::MAX).checked_add(&Duration::microseconds(1000)).is_none());

This comment has been minimized.

Sign in to view
Copy link
@lilyball

lilyball Aug 28, 2014

Contributor

This line is too long, according to make tidy.
@lilyball

This comment has been minimized.

Sign in to view
Copy link
Contributor

lilyball commented Aug 28, 2014

The failure was

/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-opt/build/src/libstd/time/duration.rs:492: line longer than 100 chars

This is indeed something you need to fix.
@lilyball

This comment has been minimized.

Sign in to view
Copy link

lilyball commented on 447b64e Aug 28, 2014

r=brson
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented on 447b64e Aug 28, 2014

saw approval from brson
at ruuda@447b64e

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Aug 28, 2014

merging ruud-v-a/rust/duration-reform = 447b64e into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Aug 28, 2014

ruud-v-a/rust/duration-reform = 447b64e merged ok, testing candidate = 1a33d7a

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Aug 28, 2014

fast-forwarding master to auto = 1a33d7a

bors added a commit that referenced this pull request Aug 28, 2014

@bors
auto merge of #16626 : ruud-v-a/rust/duration-reform, r=brson 
This changes the internal representation of `Duration` from

    days: i32,
    secs: i32,
    nanos: u32

to

    secs: i64,
    nanos: i32

This resolves #16466. Note that `nanos` is an `i32` and not `u32` as suggested, because `i32` is easier to deal with, and it is not exposed anyway. Some methods now take `i64` instead of `i32` due to the increased range. Some methods, like `num_milliseconds`, now return an `Option<i64>` instead of `i64`, because the range of `Duration` is now larger than e.g. 2^63 milliseconds.

A few remarks:
- Negating `MIN` is impossible. I chose to return `MAX` as `-MIN`, but it is one nanosecond less than the actual negation. Is this the desired behaviour?
- In `std::io::timer`, some functions accept a `Duration`, which is internally converted into a number of milliseconds. However, the range of `Duration` is now larger than 2^64 milliseconds. There is already a FIXME in the file that this should be addressed (without a ticket number though). I chose to silently use 0 ms if the duration is too long. Is that right, as long as the backend still uses milliseconds?
- Negative durations are not formatted correctly, but they were not formatted correctly before either.
Loading status checks…
1a33d7a

@bors bors closed this Aug 28, 2014

@bors bors merged commit 447b64e into rust-lang:master Aug 28, 2014
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci The Travis CI build is in progress
Details
default all tests passed

bors added a commit that referenced this pull request Aug 30, 2014

@bors
auto merge of #16650 : ruud-v-a/rust/timespec-arithmetic, r=alexcrichton 
This changes the `Add` and `Sub` implementations for `Timespec` introduced in #16573 to use `Duration` as the time span type instead of `Timespec` itself, as [suggested](#16573 (comment)) by @sfackler.

This depends on #16626, because is uses `Duration::seconds(i64)`, whereas currently `Duration::seconds` takes an `i32`.
74d9f1c

bors added a commit that referenced this pull request Aug 31, 2014

@bors
auto merge of #16650 : ruud-v-a/rust/timespec-arithmetic, r=alexcrichton 
This changes the `Add` and `Sub` implementations for `Timespec` introduced in #16573 to use `Duration` as the time span type instead of `Timespec` itself, as [suggested](#16573 (comment)) by @sfackler.

This depends on #16626, because is uses `Duration::seconds(i64)`, whereas currently `Duration::seconds` takes an `i32`.
497d54c

bors added a commit that referenced this pull request Aug 31, 2014

@bors
auto merge of #16650 : ruud-v-a/rust/timespec-arithmetic, r=alexcrichton 
This changes the `Add` and `Sub` implementations for `Timespec` introduced in #16573 to use `Duration` as the time span type instead of `Timespec` itself, as [suggested](#16573 (comment)) by @sfackler.

This depends on #16626, because is uses `Duration::seconds(i64)`, whereas currently `Duration::seconds` takes an `i32`.
5339798

bors added a commit that referenced this pull request Aug 31, 2014

@bors
auto merge of #16650 : ruud-v-a/rust/timespec-arithmetic, r=alexcrichton 
This changes the `Add` and `Sub` implementations for `Timespec` introduced in #16573 to use `Duration` as the time span type instead of `Timespec` itself, as [suggested](#16573 (comment)) by @sfackler.

This depends on #16626, because is uses `Duration::seconds(i64)`, whereas currently `Duration::seconds` takes an `i32`.
Loading status checks…
499a40e

@ruuda ruuda deleted the ruuda:duration-reform branch Sep 3, 2014

@ruuda ruuda restored the ruuda:duration-reform branch Sep 3, 2014

@ruuda ruuda deleted the ruuda:duration-reform branch Sep 3, 2014

@1-more 1-more referenced this pull request Oct 29, 2014

Closed

Yet another attempt to refactor libstd's Duration #18416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.