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

core/time: Add Duration methods for zero #72790

Merged
merged 3 commits into from
Jun 21, 2020
Merged

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented May 30, 2020

This patch adds two methods to Duration. The first, Duration::zero,
provides a const constructor for getting an zero-length duration. This
is also what Default provides (this was clarified in the docs), though
default is not const.

The second, Duration::is_zero, returns true if a Duration spans no
time (i.e., because its components are all zero). Previously, the way to
do this was either to compare both as_secs and subsec_nanos to 0, to
compare against Duration::new(0, 0), or to use the u128 method
as_nanos, none of which were particularly elegant.

@rust-highfive
Copy link
Collaborator

r? @hanna-kruppe

(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 May 30, 2020
@jonhoo
Copy link
Contributor Author

jonhoo commented May 30, 2020

I considered using "immediate" instead of "zero", but figured the latter was both shorter and (perhaps) more intuitive, even though the former is more technically accurate. Happy to change that though.

@marmeladema
Copy link
Contributor

On a similar subject, I wanted to do a PR for MIN and MAX associated const which would be very much like your zero for the MIN case I believe. Then you could just compare with MIN to replace is_zero.

@hanna-kruppe
Copy link
Contributor

Punting to randomly chosen T-libs person because I'm currently too busy: r? @LukasKalbertodt

@jonhoo
Copy link
Contributor Author

jonhoo commented May 31, 2020

@marmeladema It feels a little odd to me to have MIN for Duration and have it be zero. Because, at least in theory (though not currently in practice), a duration can be negative in length. ZERO, to me, is a distinct value from the MIN, which is the smallest representable value.

@marmeladema
Copy link
Contributor

It feels a little odd to me to have MIN for Duration and have it be zero. Because, at least in theory (though not currently in practice), a duration can be negative in length. ZERO, to me, is a distinct value from the MIN, which is the smallest representable value.

That's fair :) ZERO could be an associated const though no? I am not at all a T-libs person just a random contributor.

@jonhoo
Copy link
Contributor Author

jonhoo commented Jun 1, 2020

Oh, yes, completely agree that ZERO could instead be an associated const! I'll leave that call up to the reviewers — I don't have a strong preference either way.

@Elinvynia Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2020
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay!

I am all in for adding these methods. I am not sure if you are aware, but there was an RFC about exactly these methods. It was only closed because such a small change does not require an RFC.

ZERO as associated constant was discussed as well. I don't oppose it, but I think the zero associated function is what I would try out first. These kinds of "constructor functions" are pretty common, so I don't think adding this would hurt. The constant can still be added later.

(And for the protocol, this feature is so small that I think the doc tests are absolutely sufficient and we don't need extra tests.)

I also created a tracking issue: #73544
Please add its ID to the #[unstable] attribute.

Thanks!

src/libcore/time.rs Outdated Show resolved Hide resolved
src/libcore/time.rs Outdated Show resolved Hide resolved
This patch adds two methods to `Duration`. The first, `Duration::zero`,
provides a `const` constructor for getting an zero-length duration. This
is also what `Default` provides (this was clarified in the docs), though
`default` is not `const`.

The second, `Duration::is_zero`, returns true if a `Duration` spans no
time (i.e., because its components are all zero). Previously, the way to
do this was either to compare both `as_secs` and `subsec_nanos` to 0, to
compare against `Duration::new(0, 0)`, or to use the `u128` method
`as_nanos`, none of which were particularly elegant.
@jonhoo
Copy link
Contributor Author

jonhoo commented Jun 20, 2020

Oh, nice, didn't know about that RFC! I've revised according to your comments. Thanks for creating the tracking issue :)

@LukasKalbertodt
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 20, 2020

📌 Commit 386114b has been approved by LukasKalbertodt

@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 Jun 20, 2020
@LukasKalbertodt
Copy link
Member

CC @ghost in case you didn't see this PR implementing your RFC.

Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 20, 2020
…bertodt

core/time: Add Duration methods for zero

This patch adds two methods to `Duration`. The first, `Duration::zero`,
provides a `const` constructor for getting an zero-length duration. This
is also what `Default` provides (this was clarified in the docs), though
`default` is not `const`.

The second, `Duration::is_zero`, returns true if a `Duration` spans no
time (i.e., because its components are all zero). Previously, the way to
do this was either to compare both `as_secs` and `subsec_nanos` to 0, to
compare against `Duration::new(0, 0)`, or to use the `u128` method
`as_nanos`, none of which were particularly elegant.
This was referenced Jun 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2020
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#72456 (Try to suggest dereferences on trait selection failed)
 - rust-lang#72788 (Projection bound validation)
 - rust-lang#72790 (core/time: Add Duration methods for zero)
 - rust-lang#73227 (Allow multiple `asm!` options groups and report an error on duplicate options)
 - rust-lang#73287 (lint: normalize projections using opaque types)
 - rust-lang#73291 (Pre-compute `LocalDefId` <-> `HirId` mappings and remove `NodeId` <-> `HirId` conversion APIs)
 - rust-lang#73378 (Remove use of specialization from librustc_arena)
 - rust-lang#73411 (Update bootstrap to rustc 1.45.0-beta.2 (1dc0f6d 2020-06-15))
 - rust-lang#73443 (ci: allow gating GHA on everything but macOS)

Failed merges:

r? @ghost
@bors bors merged commit c47550f into rust-lang:master Jun 21, 2020
@jonhoo jonhoo deleted the duration-is-zero branch June 21, 2020 02:48
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?
RalfJung added a commit to RalfJung/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?
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants