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

Stabilize the Duration API #26818

Merged
merged 6 commits into from Aug 11, 2015

Conversation

Reviewers
No reviews
Assignees

@aturon aturon

Labels
T-libs beta-accepted
Projects
None yet
Milestone
No milestone
9 participants
@sfackler @rust-highfive @alexcrichton @aturon @brson @lifthrasiir @bors @steveklabnik @pnkfelix
@sfackler
Copy link
Member

sfackler commented Jul 6, 2015

This commit stabilizes the std::time module and the Duration type.
Duration::span remains unstable, and the Display implementation for
Duration has been removed as it is still being reworked and all trait
implementations for stable types are de facto stable.

This is a [breaking-change] to those using Duration's Display
implementation.

I'm opening this PR as a platform for discussion - there may be some method renaming to do as part of the stabilization process.

@sfackler sfackler added the T-libs label Jul 6, 2015

@rust-highfive rust-highfive assigned aturon Jul 6, 2015

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Jul 6, 2015

r? @aturon

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Jul 6, 2015

Internals discussion: https://internals.rust-lang.org/t/final-comment-period-for-the-duration-api/2325

@alexcrichton alexcrichton added the I-needs-decision label Jul 6, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Jul 6, 2015

I'm happy with this course of action. Could you also tag the trait implementations with the same stability tag as well?
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Jul 7, 2015

I'm likewise fine with this general direction, modulo the renamings discussion on the internals thread. Will try to weigh in there soon.
@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Jul 7, 2015

I also think it's time to get this done, and don't have opinions about the fine details - seems like this type has been well-polished at this point.

I'd definitely like to hear @lifthrasiir's opinions about this since rust-chrono depends on it.
@lifthrasiir

This comment has been minimized.

Sign in to view
Copy link
Contributor

lifthrasiir commented Jul 9, 2015

I have no particular objection, as Chrono has been a (relatively) happy user of the in-tree Duration type. I would have to adjust Chrono to deal with the removal of Display implementation but I can deal with it.

@alexcrichton alexcrichton referenced this pull request Jul 29, 2015

Closed

Rewrite <Duration as fmt::Display>::fmt() #25481

@sfackler sfackler force-pushed the sfackler:duration-stabilization branch from 3360c04 to f785f06 Aug 4, 2015

@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Aug 4, 2015

I've updated the PR to rename secs to as_secs and extra_nanos to subsec_nanos in response to feedback from the internals discussion.

@sfackler sfackler force-pushed the sfackler:duration-stabilization branch from f785f06 to 0ba159c Aug 4, 2015

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Aug 5, 2015

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

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Aug 7, 2015

Can you keep deprecated versions of the old method names? We do this even for unstable APIs to ease changes in.

Otherwise, I'm ready to r+ assuming everyone is on board backporting this stabilization to 1.3 (which has essentially no downside, AFAICT.)
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Aug 7, 2015

(which has essentially no downside, AFAICT.)

The downside is a deviation from our standard release process. Yes, it's a bit painful to just miss a train, but at the same time, I don't want us to get in the habit of it.
@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Aug 7, 2015

I'm assuming I should leave the old versions unstable and then cut them out in a release or two?
@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Aug 7, 2015

Updated

@sfackler sfackler force-pushed the sfackler:duration-stabilization branch 2 times, most recently from 5b31967 to e28e707 Aug 7, 2015

@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Aug 8, 2015

Should I change the feature for the stable functionality? Tidy's complaining:

error: feature 'duration' is both stable and unstable
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Aug 10, 2015

Yeah it's fine to just change the feature name of the deprecated methods to something new like duration_old.
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Aug 10, 2015

FYI: we're going to discuss the backporting question in the core team meeting today.

@sfackler sfackler force-pushed the sfackler:duration-stabilization branch from e28e707 to 9124dd9 Aug 10, 2015

@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Aug 10, 2015

Cool. Adjusted the feature for the old methods.
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Aug 10, 2015

We discussed this at the core team meeting and the consensus is that it's fine to backport this stabilization to 1.3 beta. In the future, we hope to avoid this by having more explicit "go/no go" subteam meetings around stabilization, sufficiently prior to the beta being cut.
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Aug 10, 2015

@bors: r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Aug 10, 2015

📌 Commit 9124dd9 has been approved by aturon

@aturon aturon added beta-nominated beta-accepted labels Aug 10, 2015

@aturon aturon removed the I-needs-decision label Aug 10, 2015

@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Aug 10, 2015

@bors: r-

(need to fix some benchmarks)

pnkfelix and others added some commits Aug 7, 2015

@pnkfelix @sfackler
Turn nonzeroing move hints back off by default. 
This is a temporary workaround for the bugs that have been found in
the implementation of PR #26173.

 * pnkfelix is unavailable in the short-term (i.e. for the next week) to fix them.

 * When the bugs are fixed, we will turn this back on by default.

(If you want to play with the known-to-be-buggy optimization in the
meantime, you can opt-back in via the debugging option that this
commit is toggling.)
91f5d32
@pnkfelix @sfackler
Regression test for dropflag reinit issue. 
Fix #27401.
fc54934
@pnkfelix @sfackler
placate the pretty tests by ignoring my test.
af32c01
@sfackler
Stabilize the Duration API 
This commit stabilizes the `std::time` module and the `Duration` type.
`Duration::span` remains unstable, and the `Display` implementation for
`Duration` has been removed as it is still being reworked and all trait
implementations for stable types are de facto stable.

This is a [breaking-change] to those using `Duration`'s `Display`
implementation.
999bdec
@sfackler
Add back and deprecate old methods.
Loading status checks…
e29a62f

@sfackler sfackler force-pushed the sfackler:duration-stabilization branch from 9124dd9 to e29a62f Aug 11, 2015

@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Aug 11, 2015

@bors r=aturon
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Aug 11, 2015

📌 Commit e29a62f has been approved by aturon
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Aug 11, 2015

⌛️ Testing commit e29a62f with merge f2790eb...

bors added a commit that referenced this pull request Aug 11, 2015

@bors
Auto merge of #26818 - sfackler:duration-stabilization, r=aturon 
This commit stabilizes the `std::time` module and the `Duration` type.
`Duration::span` remains unstable, and the `Display` implementation for
`Duration` has been removed as it is still being reworked and all trait
implementations for stable types are de facto stable.

This is a [breaking-change] to those using `Duration`'s `Display`
implementation.

I'm opening this PR as a platform for discussion - there may be some method renaming to do as part of the stabilization process.
f2790eb
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Aug 11, 2015

💔 Test failed - auto-mac-64-opt
@sfackler
Fix doc test
Loading status checks…
b51e009
@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Aug 11, 2015

@bors r=aturon
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Aug 11, 2015

📌 Commit b51e009 has been approved by aturon

bors added a commit that referenced this pull request Aug 11, 2015

@bors
Auto merge of #26818 - sfackler:duration-stabilization, r=aturon 
This commit stabilizes the `std::time` module and the `Duration` type.
`Duration::span` remains unstable, and the `Display` implementation for
`Duration` has been removed as it is still being reworked and all trait
implementations for stable types are de facto stable.

This is a [breaking-change] to those using `Duration`'s `Display`
implementation.

I'm opening this PR as a platform for discussion - there may be some method renaming to do as part of the stabilization process.
Loading status checks…
50141d7
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Aug 11, 2015

⌛️ Testing commit b51e009 with merge 50141d7...
@bors

@bors bors merged commit b51e009 into rust-lang:master Aug 11, 2015
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details

@brson brson referenced this pull request Aug 11, 2015

Merged

Beta next #27668

@alexcrichton alexcrichton removed the beta-nominated label Aug 12, 2015

@sfackler sfackler deleted the sfackler:duration-stabilization branch Nov 26, 2016

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.