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

Tracking issue for time_checked_add feature #55940

Closed
sgeisler opened this issue Nov 13, 2018 · 11 comments · Fixed by #58034
Closed

Tracking issue for time_checked_add feature #55940

sgeisler opened this issue Nov 13, 2018 · 11 comments · Fixed by #58034
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@sgeisler
Copy link
Contributor

This is a tracking issue for PR #55527 .

This feature adds a checked_add(&self, &Duration) -> Option<SystemTime> function to std::time::SystemTime and as a prerequisite also for all platform specific time structs. This also led to the refactoring of many add_duration(&self, &Duration) -> SystemTime functions to avoid redundancy (they now unwrap the result of checked_add_duration).

@sfackler sfackler added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Nov 14, 2018
@Amanieu
Copy link
Member

Amanieu commented Dec 4, 2018

This should be added to Instant as well. We're seeing some need for this in parking_lot to handle wait timeouts.

@faern
Copy link
Contributor

faern commented Dec 4, 2018

I pushed a PR (#56490) for adding the same functionality to Instant under the same feature gate name. I hope this is fine. They seem extremely related (and share a lot of the implementation) and the feature name fits both.

kennytm added a commit to kennytm/rust that referenced this issue Dec 12, 2018
…=alexcrichton

Add checked_add method to Instant time type

Appending functionality to the already opened topic of `checked_add` on time types over at rust-lang#55940.

Doing checked addition between an `Instant` and a `Duration` is important to reliably determine a future instant. We could use this in the `parking_lot` crate to compute an instant when in the future to wake a thread up without risking a panic.
bors added a commit that referenced this issue Dec 13, 2018
Add checked_add method to Instant time type

Appending functionality to the already opened topic of `checked_add` on time types over at #55940.

Doing checked addition between an `Instant` and a `Duration` is important to reliably determine a future instant. We could use this in the `parking_lot` crate to compute an instant when in the future to wake a thread up without risking a panic.
kennytm added a commit to kennytm/rust that referenced this issue Dec 14, 2018
…=alexcrichton

Add checked_add method to Instant time type

Appending functionality to the already opened topic of `checked_add` on time types over at rust-lang#55940.

Doing checked addition between an `Instant` and a `Duration` is important to reliably determine a future instant. We could use this in the `parking_lot` crate to compute an instant when in the future to wake a thread up without risking a panic.
bors added a commit that referenced this issue Dec 14, 2018
Add checked_add method to Instant time type

Appending functionality to the already opened topic of `checked_add` on time types over at #55940.

Doing checked addition between an `Instant` and a `Duration` is important to reliably determine a future instant. We could use this in the `parking_lot` crate to compute an instant when in the future to wake a thread up without risking a panic.
@faern
Copy link
Contributor

faern commented Dec 14, 2018

This has now been extended so the feature covers both checked_add and checked_sub on both Instant and SystemTime. Together with the fact that Duration already have these, it's now possible to handle all time types without risking panics on nightly (whenever the next nightly comes out).

@SimonSapin
Copy link
Contributor

Covered by this feature:

impl Instant {
    pub fn checked_add(&self, duration: Duration) -> Option<Instant> {
    pub fn checked_sub(&self, duration: Duration) -> Option<Instant> {
}
impl SystemTime {
    pub fn checked_add(&self, duration: Duration) -> Option<SystemTime> {
    pub fn checked_sub(&self, duration: Duration) -> Option<SystemTime> {
}

Looks good to me.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 1, 2019

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 1, 2019
@rfcbot
Copy link

rfcbot commented Feb 2, 2019

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

@rfcbot 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 Feb 2, 2019
@vi
Copy link
Contributor

vi commented Feb 5, 2019

Why no Instant::checked_duration_since?

@rfcbot
Copy link

rfcbot commented Feb 12, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 12, 2019
@vi
Copy link
Contributor

vi commented Feb 12, 2019

Implemented checked_duration_since: #58395

@faern
Copy link
Contributor

faern commented Feb 12, 2019

@vi Could we not keep that as a separate feature? As to not slow this one down. No one reported they might have any need for checked_duration_since when we developed checked_add and checked_sub.

@vi
Copy link
Contributor

vi commented Feb 12, 2019

OK, renaming the feature in the pull request.

Centril added a commit to Centril/rust that referenced this issue Feb 13, 2019
…=alexcrichton

Stabilize the time_checked_add feature

Closes rust-lang#55940

Stabilizes `checked_add` and `checked_sub` on `Instant` and `SystemTime`.
Centril added a commit to Centril/rust that referenced this issue Feb 13, 2019
…=alexcrichton

Stabilize the time_checked_add feature

Closes rust-lang#55940

Stabilizes `checked_add` and `checked_sub` on `Instant` and `SystemTime`.
Centril added a commit to Centril/rust that referenced this issue Feb 13, 2019
…=alexcrichton

Stabilize the time_checked_add feature

Closes rust-lang#55940

Stabilizes `checked_add` and `checked_sub` on `Instant` and `SystemTime`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants