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 Duration saturating operations #76416

Closed
3 tasks done
marmeladema opened this issue Sep 6, 2020 · 7 comments · Fixed by #84090
Closed
3 tasks done

Tracking Issue for Duration saturating operations #76416

marmeladema opened this issue Sep 6, 2020 · 7 comments · Fixed by #84090
Labels
A-time Area: Time C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@marmeladema
Copy link
Contributor

marmeladema commented Sep 6, 2020

This is a tracking issue for Duration saturating operations.
The feature gate for the issue is #![feature(duration_saturating_ops)].

        impl Duration {
            pub const fn saturating_add(self, rhs: Duration) -> Duration {}
            pub const fn saturating_sub(self, rhs: Duration) -> Duration {}
            pub const fn saturating_mul(self, rhs: u32) -> Duration {}
        }

Steps

Unresolved Questions

  • Do we want associated constants similar to the ones that already exist for integer types or associated methods? (ie: Duration::MIN or Duration::min()?
  • Should Duration::MIN be replaced by Duration::zero()? They are currently equivalent but one can image a future where Duration is able to hold negative duration and thus making MIN different from zero().
@marmeladema marmeladema added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Sep 6, 2020
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 15, 2020
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Nov 6, 2020
@matklad
Copy link
Member

matklad commented Nov 12, 2020

Just needed saturating_sub when dealing with time intervals.

Also, unresloved questions seem to be from another tracking issue?

@KodrAus KodrAus added the A-time Area: Time label Jan 6, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Mar 31, 2021

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Mar 31, 2021

Team member @m-ou-se 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 at most 2 approvals are outstanding), 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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 31, 2021
@rfcbot
Copy link

rfcbot commented Mar 31, 2021

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 31, 2021
@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 Apr 10, 2021
@rfcbot
Copy link

rfcbot commented Apr 10, 2021

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Apr 10, 2021
@marmeladema
Copy link
Contributor Author

marmeladema commented Apr 10, 2021

I am very happy to see this stabilized. One thing I wanted to discuss is the documentation: it currently mentions two associated constants:

But both are currently unstable. Should they be stabilized at the same time? Or should the documentation updated? It seems a bit weird that the documentation for stable methods is mentioning unstable features and therefore the code examples cannot be executed and verified on stable. This is really a newbie question, I don't know what is the correct approach here. I am pretty sure such circumstances have happened before?

Also, once that question is resolved, I would be very glad to do the stabilization PR. It's my first tracking issue and it would kind of be a personal accomplishment 👍

@m-ou-se
Copy link
Member

m-ou-se commented Apr 11, 2021

ZERO is being stabilized at the same time: #73544 (comment)

MAX still needs to be stabilized. Here it's suggested that that constant should probably not be part of the same tracking issue as the other constants there (SECOND, etc.). Feel free to send a PR that stabilizes just MAX, then we can do an FCP on that PR and we'll remove it from that tracking issue.

Also, once that question is resolved, I would be very glad to do the stabilization PR.

Feel free to make that PR without waiting for MAX. In this specific case I don't think it's a problem if this gets stabilized while the docs still refer to something unstable. MAX is pretty clear and uncontroversial. Even if it's unstable, it's still the best way we have to refer to the maximum value of a Duration. (And it'll probably be stable anyway in ~two weeks.)

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 11, 2021
…rating-ops, r=m-ou-se

Stabilize feature `duration_saturating_ops`

FCP here: rust-lang#76416 (comment)

Closes rust-lang#76416

r? `@m-ou-se`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 12, 2021
…ting-ops, r=m-ou-se

Stabilize feature `duration_saturating_ops`

FCP here: rust-lang#76416 (comment)

Closes rust-lang#76416

r? `@m-ou-se`
@bors bors closed this as completed in 7d89148 Apr 12, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 15, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this issue Apr 26, 2021
…ax, r=m-ou-se

Stabilize Duration::MAX

Following the suggested direction from rust-lang#76416 (comment), this PR proposes that `Duration::MAX` should have been part of the `duration_saturating_ops` feature flag all along, having been

0. heavily referenced by that feature flag
1. an odd duck next to most of `duration_constants`, as I expressed in rust-lang#57391 (comment)
2. introduced in rust-lang#76114 which added `duration_saturating_ops`

and accordingly should be folded into `duration_saturating_ops` and therefore stabilized.

r? `@m-ou-se`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-time Area: Time C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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. Libs-Tracked Libs issues that are tracked on the team's project board. 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