Skip to content
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

std: Redesign Duration, implementing RFC 1040 #24920

Merged
merged 1 commit into from May 14, 2015

Conversation

@alexcrichton
Copy link
Member

commented Apr 29, 2015

This commit is an implementation of RFC 1040 which is a redesign of the
currently-unstable Duration type. The API of the type has been scaled back to
be more conservative and it also no longer supports negative durations.

The inner duration module of the time module has now been hidden (as
Duration is reexported) and the feature name for this type has changed from
std_misc to duration. All APIs accepting durations have also been audited to
take a more flavorful feature name instead of std_misc.

Closes #24874

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2015

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2015

r? @aturon

cc @wycats

The only "surprise" I had during the implementation was the naming of the methods here and there. Abbreviating "seconds" as "secs" didn't feel super rustic to me, but typing millis/nanos felt much better than milliseconds/nanoseconds. Overall I didn't feel too strongly either way!

@rust-highfive rust-highfive assigned aturon and unassigned pcwalton Apr 29, 2015

@alexcrichton alexcrichton force-pushed the alexcrichton:duration branch 4 times, most recently from c0d3aa8 to 260ce79 Apr 29, 2015

let seconds = self.secs;
let millis = self.nanos / NANOS_PER_MILLI;
let nanos = self.nanos % NANOS_PER_MILLI;
match (seconds, millis, nanos) {

This comment has been minimized.

Copy link
@huonw

huonw Apr 29, 2015

Member

What's the motivation for this sort of 'complicated' formatting rather than something like, say, 1234.00000567s?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 29, 2015

Author Member

I'm not particularly wed to this format, but it's what was specified in the RFC (as a starting point). I also had some reservations and wouldn't mind changing it to be only numeric as well.

This comment has been minimized.

Copy link
@aturon

aturon May 4, 2015

Member

Hm, I also have mixed feelings about this actually looking at this code (I should have raised this with the RFC). I think that automatically figuring out whether to use seconds, millis, or nanos is a good idea, but I don't understand the motivation for XX seconds and XX nanoseconds.

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2015

☔️ The latest upstream changes (presumably #24888) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton alexcrichton force-pushed the alexcrichton:duration branch from 260ce79 to c30db14 Apr 29, 2015

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2015

☔️ The latest upstream changes (presumably #24967) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton alexcrichton force-pushed the alexcrichton:duration branch from c30db14 to d0990f7 Apr 30, 2015

@@ -57,25 +57,20 @@ impl Condvar {
// https://github.com/llvm-mirror/libcxx/blob/release_35/src/condition_variable.cpp#L46
// https://github.com/llvm-mirror/libcxx/blob/release_35/include/__mutex_base#L367
pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool {
if dur <= Duration::zero() {

This comment has been minimized.

Copy link
@aturon

aturon May 4, 2015

Member

This changes the behavior for a zero duration; intended?

/// the specified duration has been reached (may wake spuriously).
///
/// The semantics of this function are equivalent to `park()` except that the
/// thread will be blocked for roughly no longer than *duration*. This method

This comment has been minimized.

Copy link
@aturon

aturon May 4, 2015

Member

I think you want dur not duration.

@@ -151,6 +152,25 @@ fn cvt<I: PartialEq + Zero>(i: I) -> io::Result<I> {
}
}

fn dur2timeout(dur: Duration) -> libc::DWORD {

This comment has been minimized.

Copy link
@aturon

aturon May 4, 2015

Member

Hm, this doesn't quite match any of the proposals floated in e.g. the timeout RFC (where zero was special-cased). I wonder if just rounding up would be simpler? We should discuss.

nanos += NANOS_PER_SEC;
secs -= 1;
}
let mut secs = match self.secs.checked_sub(rhs.secs) {

This comment has been minimized.

Copy link
@aturon

aturon May 4, 2015

Member

Should this be debug only (underflow detection)?

}
/// Returns the nanosecond precision represented by this duration.
///
/// This method does **not** return the length of the duration when

This comment has been minimized.

Copy link
@aturon

aturon May 4, 2015

Member

The stern warning here makes me thing we may ultimately want to call this extra_nanos or something like that.


println!("Sent {} messages in {} ms", num_msgs, dur.num_milliseconds());

This comment has been minimized.

Copy link
@aturon

aturon May 4, 2015

Member

Hm, why change the output?

This comment has been minimized.

Copy link
@nagisa

nagisa May 9, 2015

Contributor

Because there’s no num_mulliseconds anymore and Duration has a proper Display impl now.

@@ -55,7 +55,7 @@ fn main() {
let maxf = max as f64;

println!("insert(): {} seconds\n", checkf);
println!(" : {} op/ms\n", maxf / checkf.num_milliseconds() as f64);

This comment has been minimized.

Copy link
@aturon

aturon May 4, 2015

Member

Hm, why change the output?

@aturon

This comment has been minimized.

Copy link
Member

commented May 4, 2015

@alexcrichton Ok, I've done a preliminary pass. Looks nice a simple. I raised a few concerns (most of which, to be honest, I should have raised on the RFC).

@alexcrichton alexcrichton force-pushed the alexcrichton:duration branch from d0990f7 to b59f968 May 7, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 7, 2015

Ok I have updated some feedback and discussion with @aturon:

  • Platforms now round up the duration instead of rounding down if they do not support nanosecond precision.
  • The Display implementation has become a little less wordy
  • Duration::nanos was renamed to Duration::extra_nanos. An alternative would be one method accessor returning (u64, u32), but it's unclear what the name for this accessor would be.
  • Arithmetic on Duration is now always checked.

re-r? @aturon

@alexcrichton alexcrichton force-pushed the alexcrichton:duration branch 2 times, most recently from 2fe4199 to f143b01 May 7, 2015

return Thread::yield_now()
}
let seconds = dur.num_seconds();
let ns = dur - Duration::seconds(seconds);
let mut ts = libc::timespec {

This comment has been minimized.

Copy link
@nagisa

nagisa May 9, 2015

Contributor

There should exist From<Duration> for libc::timespec in liblibc instead.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton May 11, 2015

Author Member

Unfortunately this can't be provided in liblibc because Duration doesn't exist by that point and due to the in-tree liblibc being different from the out-of-tree liblibc it unfortunately also wouldn't work to provide this in the standard library as well.

Duration::nanoseconds(diff * info.numer as i64 / info.denom as i64)
let diff = self.t as u64 - other.t as u64;
let nanos = diff * info.numer as u64 / info.denom as u64;
Duration::new(nanos / 1_000_000_000, (nanos % 1_000_000_000) as u32)

This comment has been minimized.

Copy link
@nagisa

nagisa May 9, 2015

Contributor

How about using that constant from line 70 (which should be outside of impl block anyway)?

@@ -151,6 +152,27 @@ fn cvt<I: PartialEq + Zero>(i: I) -> io::Result<I> {
}
}

fn dur2timeout(dur: Duration) -> libc::DWORD {
// Note that a duration is a (u64, u32) (seconds, nanoseconds) pair, and the

This comment has been minimized.

Copy link
@nagisa

nagisa May 9, 2015

Contributor

Hopefully nobody should have to take care of this themselves.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton May 11, 2015

Author Member

Can you elaborate a bit on this? I'm not quite sure what you're implying.

This comment has been minimized.

Copy link
@nagisa

nagisa May 11, 2015

Contributor

I’m implying that there should eventually exist a method to do such conversion on the Duration itself and that internal Duration representation shouldn’t be really relied on the end-user side (even if through the two methods).

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton May 11, 2015

Author Member

Ah yes that definitely makes sense. We don't currently have much interoperation between low-level types and standard library types in terms of conversions like this, but I can definitely see this being a windows extension trait, for example.

@@ -12,7 +12,7 @@ use ops::Sub;
use time::Duration;
use sync::{Once, ONCE_INIT};

const NANOS_PER_SEC: i64 = 1_000_000_000;
const NANOS_PER_SEC: u64 = 1_000_000_000;

This comment has been minimized.

Copy link
@nagisa

nagisa May 9, 2015

Contributor

Why that NANOS_PER_SEC constant became u32 and this became u64?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton May 11, 2015

Author Member

It was just generally what ended up allowing for the fewest casts

@alexcrichton alexcrichton force-pushed the alexcrichton:duration branch 2 times, most recently from 1b17654 to 086fddc May 11, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 11, 2015

Thanks for taking a look @nagisa!

@aturon

This comment has been minimized.

Copy link
Member

commented May 13, 2015

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2015

📌 Commit 086fddc has been approved by aturon

@aturon

This comment has been minimized.

Copy link
Member

commented May 13, 2015

Thanks @alexcrichton!

@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2015

⌛️ Testing commit 086fddc with merge 9eb94a8...

bors added a commit that referenced this pull request May 13, 2015
Auto merge of #24920 - alexcrichton:duration, r=aturon
This commit is an implementation of [RFC 1040][rfc] which is a redesign of the
currently-unstable `Duration` type. The API of the type has been scaled back to
be more conservative and it also no longer supports negative durations.

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1040-duration-reform.md

The inner `duration` module of the `time` module has now been hidden (as
`Duration` is reexported) and the feature name for this type has changed from
`std_misc` to `duration`. All APIs accepting durations have also been audited to
take a more flavorful feature name instead of `std_misc`.

Closes #24874
@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2015

💔 Test failed - auto-mac-64-nopt-t

@Manishearth

This comment has been minimized.

Copy link
Member

commented May 13, 2015

failures:

---- sync::condvar::tests::wait_timeout_with stdout ----
    thread 'sync::condvar::tests::wait_timeout_with' panicked at 'overflow when subtracting durations', /Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-nopt-t/build/src/libcore/option.rs:330



failures:
    sync::condvar::tests::wait_timeout_with

@alexcrichton alexcrichton force-pushed the alexcrichton:duration branch 2 times, most recently from cd06e02 to 38cfc7a May 13, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 13, 2015

@bors: r=aturon 38cfc7a

bors added a commit that referenced this pull request May 13, 2015
Auto merge of #24920 - alexcrichton:duration, r=aturon
This commit is an implementation of [RFC 1040][rfc] which is a redesign of the
currently-unstable `Duration` type. The API of the type has been scaled back to
be more conservative and it also no longer supports negative durations.

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1040-duration-reform.md

The inner `duration` module of the `time` module has now been hidden (as
`Duration` is reexported) and the feature name for this type has changed from
`std_misc` to `duration`. All APIs accepting durations have also been audited to
take a more flavorful feature name instead of `std_misc`.

Closes #24874
@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2015

⌛️ Testing commit 38cfc7a with merge 9b165c0...

@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2015

💔 Test failed - auto-win-64-opt

std: Redesign Duration, implementing RFC 1040
This commit is an implementation of [RFC 1040][rfc] which is a redesign of the
currently-unstable `Duration` type. The API of the type has been scaled back to
be more conservative and it also no longer supports negative durations.

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1040-duration-reform.md

The inner `duration` module of the `time` module has now been hidden (as
`Duration` is reexported) and the feature name for this type has changed from
`std_misc` to `duration`. All APIs accepting durations have also been audited to
take a more flavorful feature name instead of `std_misc`.

Closes #24874

@alexcrichton alexcrichton force-pushed the alexcrichton:duration branch from 38cfc7a to 556e76b May 14, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 14, 2015

@bors: r=aturon 556e76b

bors added a commit that referenced this pull request May 14, 2015
Auto merge of #24920 - alexcrichton:duration, r=aturon
This commit is an implementation of [RFC 1040][rfc] which is a redesign of the
currently-unstable `Duration` type. The API of the type has been scaled back to
be more conservative and it also no longer supports negative durations.

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1040-duration-reform.md

The inner `duration` module of the `time` module has now been hidden (as
`Duration` is reexported) and the feature name for this type has changed from
`std_misc` to `duration`. All APIs accepting durations have also been audited to
take a more flavorful feature name instead of `std_misc`.

Closes #24874
@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2015

⌛️ Testing commit 556e76b with merge 0755c09...

@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2015

💔 Test failed - auto-linux-32-nopt-t

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 14, 2015

@bors: retry

On Wed, May 13, 2015 at 8:36 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-32-nopt-t
http://buildbot.rust-lang.org/builders/auto-linux-32-nopt-t/builds/4942


Reply to this email directly or view it on GitHub
#24920 (comment).

@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2015

⌛️ Testing commit 556e76b with merge dd4dad8...

bors added a commit that referenced this pull request May 14, 2015
Auto merge of #24920 - alexcrichton:duration, r=aturon
This commit is an implementation of [RFC 1040][rfc] which is a redesign of the
currently-unstable `Duration` type. The API of the type has been scaled back to
be more conservative and it also no longer supports negative durations.

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1040-duration-reform.md

The inner `duration` module of the `time` module has now been hidden (as
`Duration` is reexported) and the feature name for this type has changed from
`std_misc` to `duration`. All APIs accepting durations have also been audited to
take a more flavorful feature name instead of `std_misc`.

Closes #24874

@bors bors merged commit 556e76b into rust-lang:master May 14, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
zmbush added a commit to zmbush/megaprompt that referenced this pull request May 18, 2015

@alexcrichton alexcrichton deleted the alexcrichton:duration branch Aug 17, 2015

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