Move Duration to libcore #46666

clarfonthey · 2017-12-11T18:43:06Z

Fixes #46520; should be merged after #46508.

rust-highfive · 2017-12-11T18:43:18Z

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

malbarbo · 2017-12-12T17:42:30Z

src/libcore/time.rs

+    ///
+    /// let duration = Duration::from_millis(5432);
+    /// assert_eq!(duration.as_secs(), 5);
+    /// assert_eq!(duration.subsec_nanos(), 432_000_000);


subsec_millis?

This is part of #46508

malbarbo · 2017-12-12T17:42:56Z

src/libcore/time.rs

+    ///
+    /// let duration = Duration::from_micros(1_234_567);
+    /// assert_eq!(duration.as_secs(), 1);
+    /// assert_eq!(duration.subsec_nanos(), 234_567_000);


subsec_micros?

This is part of #46508

kennytm · 2017-12-20T14:38:29Z

#46508 has been merged. @clarcharr

bors · 2017-12-20T17:36:50Z

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

clarfonthey · 2017-12-21T19:02:13Z

@kennytm Just rebased! Waiting on CI.

sfackler · 2017-12-21T19:05:03Z

src/libcore/time.rs

@@ -7,6 +7,19 @@
 // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
+#![stable(feature = "duration", since = "1.3.0")]


Should the module level stability here be updated to 1.24.0 with a new feature name?

Not sure! My assumption is that because we're simply moving what was already stable in std to core, that we should use the same feature and version. But I can update it or mark it as unstable if desired.

The type's stability definitely shouldn't change, but its placement in libcore is new. If nothing else, it's a nice indication in rustdoc as to when it first showed up.

@aturon what do you think?

@sfackler makes sense! considering how libstd will always show up on docs searches before libcore, this should help. I'll make this change and then wait for @aturon's response.

kennytm · 2017-12-21T19:40:12Z

src/libcore/time.rs

@@ -45,7 +58,7 @@ const MICROS_PER_SEC: u64 = 1_000_000;
 ///
 /// let ten_millis = Duration::from_millis(10);
 /// ```
-#[stable(feature = "duration", since = "1.3.0")]
+#![stable(feature = "duration_core", since = "1.24.0")]


Why add the ! here...

[00:03:27] error: an inner attribute is not permitted following an outer doc comment [00:03:27] --> /checkout/src/libcore/time.rs:61:3 [00:03:27] | [00:03:27] 61 | #![stable(feature = "duration_core", since = "1.24.0")] [00:03:27] | ^ [00:03:27] | [00:03:27] = note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files. Outer attributes, like `#[test]`, annotate the item following them.

…I copy-pasted and shouldn't have.

clarfonthey · 2017-12-21T23:40:05Z

(it is passing now)

bors · 2017-12-22T01:51:19Z

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

sfackler · 2017-12-23T06:55:06Z

@bors r+

bors · 2017-12-23T06:55:06Z

📌 Commit 3f6b8e1 has been approved by sfackler

sfackler · 2017-12-23T06:55:34Z

@bors r-

sfackler · 2017-12-23T06:55:58Z

Er, actually, let's make sure the rest of the libs folks are okay with this since the move is insta-stable.

@rfcbot fcp merge

rfcbot · 2017-12-23T06:55:59Z

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

~~we-may-want-std~~ resolved by Move Duration to libcore #46666 (comment)

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

clarfonthey · 2018-01-12T22:58:59Z

@alexcrichton I mean, that PR is two years old and Rust has gained a lot of no_std crates in that timeframe. I don't really know what sort of examples I can give, other than stating that time measurement is something that shows up in embedded systems all the time. If I wanted to code a simple OS in Rust I'd have to do all time measurement with a custom Duration struct, or probably just an int measuring nanoseconds, which seems unnecessary considering how all that work is already done in the standard library.

Pre-1.0 I actually used a bit of Rust for a systems programming class, and although Duration didn't exist at the time, if I were to code such a thing now I'd definitely be a bit upset that this is included in libstd despite definitely being a system-agnostic type.

To flip the question: do you have any vague idea of what an std-requiring Duration would look like? What kind of functionality would need to be system-specific and not included in a separate type like SystemTime or Instant?

(to add: if Duration represented a moment in time I would absolutely agree with you, but it represents the distance between two points in time, which is much different)

bors · 2018-01-12T23:28:53Z

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

alexcrichton · 2018-01-16T20:19:58Z

Ah sorry I meant to comment on this but forgot! The @rust-lang/libs team discussed this PR at triage last week and the conclusion was that we should go ahead and move forward with it. Our thinking was that we can't think of any functionality we'd really like to add to Duration any time soon which requires libstd, and if this does come about we can always figure out a solution. In the meantime Duration also has a pro in that it's so simple, so it seems fine to include in libcore.

@clarcharr want to rebase before merging?

@rfcbot fcp merge

alexcrichton · 2018-01-16T20:20:55Z

Also can you move the tests to an the src/libcore/tests folder? The inline tests in the module won't be run I believe.

kennytm · 2018-01-16T21:02:26Z

rfcbot is not responding 😕

alexcrichton · 2018-01-16T22:15:36Z

er sorry that's my mistake, it was already done above!

@rfcbot resolved we-may-want-std

rfcbot · 2018-01-16T22:17:19Z

🔔 This is now entering its final comment period, as per the review above. 🔔

clarfonthey · 2018-01-24T00:24:07Z

Just realised that one primary use of libstd Duration is to have a static method which times the duration a function takes to execute, and imo that would be better suited for the to-be-stabled Bencher from libtest

alexcrichton · 2018-01-25T00:37:58Z

That was an example of what we felt wasn't necessarily critical functionality to have but something we could stabilize at a later date via other means. How does that sound to you though?

clarfonthey · 2018-01-25T00:40:18Z

Seems fine to me!

Mark-Simulacrum · 2018-01-25T00:53:20Z

So I believe the consensus here is we want to merge this, but are waiting on a rebase. Is that correct?

alexcrichton · 2018-01-25T01:01:45Z

Indeed!

rfcbot · 2018-01-26T22:18:59Z

The final comment period is now complete.

clarfonthey · 2018-01-29T23:17:20Z

Rebased!

alexcrichton · 2018-01-30T15:08:16Z

@bors: r+

bors · 2018-01-30T15:08:17Z

📌 Commit aab712c has been approved by alexcrichton

bors · 2018-01-31T04:16:18Z

⌛ Testing commit aab712c with merge b8f2674...

Move Duration to libcore Fixes #46520; should be merged after #46508.

bors · 2018-01-31T06:59:07Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b8f2674 to master...

rust-highfive assigned sfackler Dec 11, 2017

kennytm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 11, 2017

clarfonthey force-pushed the duration_core branch 2 times, most recently from 483a3cd to ab0b5a8 Compare December 12, 2017 16:31

malbarbo reviewed Dec 12, 2017

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Dec 20, 2017

clarfonthey force-pushed the duration_core branch from ab0b5a8 to 94113b7 Compare December 21, 2017 19:00

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 21, 2017

sfackler reviewed Dec 21, 2017

View reviewed changes

kennytm reviewed Dec 21, 2017

View reviewed changes

clarfonthey force-pushed the duration_core branch from c876661 to 09b6a5b Compare December 21, 2017 20:17

clarfonthey force-pushed the duration_core branch from 09b6a5b to 3f6b8e1 Compare December 22, 2017 22:57

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 23, 2017

kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 23, 2017

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 16, 2018

kennytm removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 16, 2018

Move time::Duration to libcore

aab712c

clarfonthey force-pushed the duration_core branch from 3f6b8e1 to aab712c Compare January 29, 2018 23:17

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 30, 2018

bors added a commit that referenced this pull request Jan 31, 2018

Auto merge of #46666 - clarcharr:duration_core, r=alexcrichton

b8f2674

Move Duration to libcore Fixes #46520; should be merged after #46508.

bors merged commit aab712c into rust-lang:master Jan 31, 2018

bors mentioned this pull request Jan 31, 2018

Add std::time::Duration::{from_days, from_hours, from_mins} #47097

Closed

clarfonthey deleted the duration_core branch April 15, 2018 20:31

elichai mentioned this pull request Nov 6, 2019

[Discussion] Updating MSRV to 1.29.0 rust-bitcoin/rust-bitcoin#338

Closed

Move Duration to libcore #46666

Move Duration to libcore #46666

Conversation

clarfonthey commented Dec 11, 2017

rust-highfive commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm commented Dec 20, 2017

bors commented Dec 20, 2017

clarfonthey commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarfonthey commented Dec 21, 2017

bors commented Dec 22, 2017

sfackler commented Dec 23, 2017

bors commented Dec 23, 2017

sfackler commented Dec 23, 2017

sfackler commented Dec 23, 2017

rfcbot commented Dec 23, 2017 • edited by alexcrichton Loading

clarfonthey commented Jan 12, 2018 • edited Loading

bors commented Jan 12, 2018

alexcrichton commented Jan 16, 2018

alexcrichton commented Jan 16, 2018

kennytm commented Jan 16, 2018

alexcrichton commented Jan 16, 2018

rfcbot commented Jan 16, 2018

clarfonthey commented Jan 24, 2018

alexcrichton commented Jan 25, 2018

clarfonthey commented Jan 25, 2018

Mark-Simulacrum commented Jan 25, 2018

alexcrichton commented Jan 25, 2018

rfcbot commented Jan 26, 2018

clarfonthey commented Jan 29, 2018

alexcrichton commented Jan 30, 2018

bors commented Jan 30, 2018

bors commented Jan 31, 2018

bors commented Jan 31, 2018

rfcbot commented Dec 23, 2017 •

edited by alexcrichton

Loading

clarfonthey commented Jan 12, 2018 •

edited

Loading