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

Add saturating methods for Duration #76114

Merged
merged 1 commit into from
Sep 12, 2020

Conversation

marmeladema
Copy link
Contributor

@marmeladema marmeladema commented Aug 30, 2020

In some project, I needed a saturating_add method for Duration. I implemented it myself but i thought it would be a nice addition to the standard library as it matches closely with the integers types.

3 new methods have been introduced and are gated by the new duration_saturating_ops unstable feature:

  • Duration::saturating_add
  • Duration::saturating_sub
  • Duration::saturating_mul

If have left the tracking issue to none for now as I want first to understand if those methods would be acceptable at all. If agreed, I'll update the PR with the tracking issue.

Further more, to match the behavior of integers types, I introduced 2 associated constants:

Both have been gated by the already existing unstable feature duration_constants, I can introduce a new unstable feature if needed or just re-use the duration_saturating_ops.

We might have to decide whether:

  • MIN should be replaced by ZERO?
  • associated constants over methods?

@rust-highfive
Copy link
Collaborator

r? @shepmaster

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2020
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 30, 2020
@marmeladema marmeladema force-pushed the duration-saturating-ops branch from 044ca51 to 904ec9f Compare August 30, 2020 19:20
library/core/src/time.rs Outdated Show resolved Hide resolved
Copy link
Member

@shepmaster shepmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits.

library/core/src/time.rs Show resolved Hide resolved
library/core/src/time.rs Show resolved Hide resolved
library/core/src/time.rs Show resolved Hide resolved
src/test/ui/consts/duration-consts-2.rs Show resolved Hide resolved
library/core/src/time.rs Outdated Show resolved Hide resolved
@marmeladema marmeladema closed this Sep 4, 2020
@shepmaster
Copy link
Member

Why the PR close?

@marmeladema
Copy link
Contributor Author

@shepmaster I really don't know what happened. I never meant to close this!

@marmeladema
Copy link
Contributor Author

@shepmaster @pickfire I think i've addressed all the comments 👍

library/core/src/time.rs Outdated Show resolved Hide resolved
library/core/src/time.rs Outdated Show resolved Hide resolved
library/core/src/time.rs Outdated Show resolved Hide resolved
library/core/src/time.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me except the mentioned doc changes.

@marmeladema marmeladema force-pushed the duration-saturating-ops branch from 30aa7dd to b869aa5 Compare September 7, 2020 23:43
@marmeladema
Copy link
Contributor Author

@pickfire I've fixed the docs according to your comments 👍

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@shepmaster
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 11, 2020

📌 Commit b869aa5 has been approved by shepmaster

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 12, 2020
…, r=shepmaster

Add saturating methods for `Duration`

In some project, I needed a `saturating_add` method for `Duration`. I implemented it myself but i thought it would be a nice addition to the standard library as it matches closely with the integers types.

3 new methods have been introduced and are gated by the new `duration_saturating_ops` unstable feature:
* `Duration::saturating_add`
* `Duration::saturating_sub`
* `Duration::saturating_mul`

If have left the tracking issue to `none` for now as I want first to understand if those methods would be acceptable at all. If agreed, I'll update the PR with the tracking issue.

Further more, to match the behavior of integers types, I introduced 2 associated constants:
* `Duration::MIN`: this one is somehow a duplicate from `Duration::zero()` method, but at the time this method was added, `MIN` was rejected as it was considered a different semantic (see rust-lang#72790 (comment)).
* `Duration::MAX`

Both have been gated by the already existing unstable feature `duration_constants`, I can introduce a new unstable feature if needed or just re-use the `duration_saturating_ops`.

We might have to decide whether:
* `MIN` should be replaced by `ZERO`?
* associated constants over methods?
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#76114 (Add saturating methods for `Duration`)
 - rust-lang#76297 (rustdoc: fix min_const_generics with ty::Param)
 - rust-lang#76484 (Add MaybeUninit::assume_init_drop.)
 - rust-lang#76530 (Eliminate mut reference UB in Drop impl for Rc<T>)
 - rust-lang#76583 (Update `std::os` module documentation.)
 - rust-lang#76599 (Finish off revisions for const generics UI tests.)
 - rust-lang#76615 (Add missing examples on binary core traits)

Failed merges:

r? `@ghost`
@bors bors merged commit 7344f93 into rust-lang:master Sep 12, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 12, 2020
@marmeladema marmeladema deleted the duration-saturating-ops branch September 15, 2020 17:46
m-ou-se added a commit to m-ou-se/rust that referenced this pull request 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
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants