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

Duration::zero() -> Duration::ZERO #78216

Merged
merged 4 commits into from
Nov 12, 2020
Merged

Conversation

workingjubilee
Copy link
Contributor

In review for #72790, whether or not a constant or a function should be favored for #![feature(duration_zero)] was seen as an open question. In #73544 (comment) an invitation was opened to either stabilize the methods or propose a switch to the constant value, supplemented with reasoning. Followup comments suggested community preference leans towards the const ZERO, which would be reason enough.

ZERO also "makes sense" beside existing associated consts for Duration. It is ever so slightly awkward to have a series of constants specifying 1 of various units but leave 0 as a method, especially when they are side-by-side in code. It seems unintuitive for the one non-dynamic value (that isn't from Default) to be not-a-const, which could hurt discoverability of the associated constants overall. Elsewhere in std, methods for obtaining a constant value were even deprecated, as seen with std::u32::min_value.

Most importantly, ZERO costs less to use. A match supports a const pattern, but const fn can only be used if evaluated through a const context such as an inline const { const_fn() } or a const NAME: T = const_fn() declaration elsewhere. Likewise, while #73544 (comment) notes Duration::zero() can optimize to a constant value, "can" is not "will". Only const contexts have a strong promise of such. Even without that in mind, the comment in question still leans in favor of the constant for simplicity. As it costs less for a developer to use, may cost less to optimize, and seems to have more of a community consensus for it, the associated const seems best.

r? @LukasKalbertodt

This expands time's test suite to use more and in more places the
range of methods and constants added to Duration in recent
proposals for the sake of testing more API surface area and
improving legibility.
Duration::ZERO composes better with match and various other things,
at the cost of an occasional parens, and results in less work for the
optimizer, so let's use that instead.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2020
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 22, 2020
@LukasKalbertodt
Copy link
Member

Makes sense to me! As I said in my comment you linked: I think either solution is fine, so moving forward with this seems like a good idea!

Reassigning to start FCP:
r? @Amanieu

@LukasKalbertodt
Copy link
Member

Woopsie, this is not a stabilization PR yet 😄 Sorry for the accidental ping.

r? @LukasKalbertodt

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.

Overall looks good to me, just a few minor nits.

Also, #76114 introduce Duration::MIN. We probably want to replace it with ZERO, but we can always do that later.

library/core/tests/time.rs Outdated Show resolved Hide resolved
library/core/tests/time.rs Outdated Show resolved Hide resolved
library/core/tests/time.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Contributor Author

Duration::MIN is exactly why I figured I didn't want to open a stabilization PR for this just yet, because whether or not one should override or the two should alias was uncertain to me and I felt that should be an open question, and I felt the community should get half a second to respond to this change arriving in nightly. If you are in favor of removing Duration::MIN in favor of Duration::ZERO, then I will include that in this PR as another commit so it can be reverted individually.

Duration::ZERO supercedes it in effect.
@m-ou-se
Copy link
Member

m-ou-se commented Nov 11, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Nov 11, 2020

📌 Commit 82f3a23 has been approved by m-ou-se

@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 Nov 11, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 11, 2020

MIN was introduced very recently, so replacing it should not be much of a problem.

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2020
…as-schievink

Rollup of 11 pull requests

Successful merges:

 - rust-lang#78216 (Duration::zero() -> Duration::ZERO)
 - rust-lang#78354 (Support enable/disable sanitizers/profiler per target)
 - rust-lang#78417 (BTreeMap: split off most code of append)
 - rust-lang#78832 (look at assoc ct, check the type of nodes)
 - rust-lang#78873 (Add flags customizing behaviour of MIR inlining)
 - rust-lang#78899 (Support inlining diverging function calls)
 - rust-lang#78923 (Cleanup and comment intra-doc link pass)
 - rust-lang#78929 (rustc_target: Move target env "gnu" from `linux_base` to `linux_gnu_base`)
 - rust-lang#78930 (rustc_taret: Remove `TargetOptions::is_like_android`)
 - rust-lang#78942 (Fix typo in comment)
 - rust-lang#78947 (Ship llvm-cov through llvm-tools)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@marmeladema
Copy link
Contributor

Hello!
I am the one who introduced MIN. The rational at the time for not using ZERO was that potentially Duration could hold negative duration in the future and at this point MIN would be different from ZERO. If we are confident that will never happen there is no problem for me to replace one with the other.

@bors bors merged commit 62f0a78 into rust-lang:master Nov 12, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 12, 2020
@workingjubilee
Copy link
Contributor Author

A "negative Duration" doesn't make much sense to me, but that's for libs to decide, I think.

@m-ou-se
Copy link
Member

m-ou-se commented Nov 12, 2020

@marmeladema Ah, thanks for adding that note here. It's useful to have that context.

Duration will most likely never allow for negative values, as that'd be a breaking change. Functions like checked_sub already promise to return None if the result would have been negative.

@faern
Copy link
Contributor

faern commented Nov 12, 2020

A "negative Duration" doesn't make much sense to me, but that's for libs to decide, I think.

I fully agree with this. And even if there were negative durations, I don't think it would be immediately apparent that a negative duration would be "smaller" than a duration of time zero. I don't think it makes much sense to talk about a "minimal" duration other than zero, and then the name zero makes way more sense.

I also don't think it would be good to change the value of this type of constant after it had been stabilized. If MIN were to be stabilized as a duration of zero time there would sure be people using it to mean zero time. If it was later changed to negative 1000 years or something I'm pretty sure any code using it would break like crazy. Are there any prior examples in Rust where a stabilized constant changed value in a later release? (I'm of course in favor of fixing values that were wrong or if a mathematical constant can be expressed with higher precision or something. But that's different)

@polomsky
Copy link

polomsky commented Feb 14, 2021

Hello. I would like to add few notes about duration.
I see both use cases.

  1. I need some value for thread::sleep and similar methods - clearly negative duration makes no sense here and should
    a. be impossible
    b. cause panic
    c. behave as zero duration (e.g. this actually happen in golang)
  2. And there is another kind of duration, when I want calculate difference between two timestamps surely here I need negative durations. About problem about MIN duration can be solved the same way as for real numbers. There is smallest positive non zero value, which could be applied to duration too if needed, and similar value exists for negative real number - so could be also for duration - but for "smallest negative" duration (negative and closest to zero) I see no usecase.

These two usecases can be covered by single type in case of 1.b or 1.c (preferably c) or by two types, in case of 1.a

To prevent BC break, another type should be introduced, e.g. TimeDifference which some nice conversion to Duration and back.

@m-ou-se
Copy link
Member

m-ou-se commented Feb 14, 2021

@polomsky New comments on already closed/merged PRs are easy to miss. If you found an issue with something in the language or libary, it'd be good to open a new issue for it instead. For new ideas, it's usually best to discuss it on the internals forum first.

@workingjubilee workingjubilee deleted the duration-zero branch April 23, 2021 19:21
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.

None yet