core/time: Add Duration methods for zero by jonhoo · Pull Request #72790 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

jonhoo

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

r? @hanna-kruppe

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

@jonhoo

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

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

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

@jonhoo

@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

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

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.

LukasKalbertodt

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!

@jonhoo

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

@jonhoo

@jonhoo

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

LukasKalbertodt

@LukasKalbertodt

@bors

📌 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

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

@Manishearth

…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

@bors

…arth

Rollup of 9 pull requests

Successful merges:

Failed merges:

r? @ghost

@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

@Dylan-DPC

…, 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:

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:

RalfJung added a commit to RalfJung/rust that referenced this pull request

Sep 12, 2020

@RalfJung

…, 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:

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:

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.