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 Duration::to_nanos #35868

Closed
wants to merge 1 commit into from

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Aug 20, 2016

Sometimes it is useful to extract a single number from a Duration, with better precision than as_secs. Overflow is possible but in many situation very unlikely. Of course users can do the math themselves with as_secs and subsec_nanos, but it’s not as convenient and there’s a risk of using an incorrect number of zeros in the conversion. (I have written a bug much like that at least once.)

This adds a Duration::to_nanos(&self) -> Option<u64> method. Result could be used instead with a new OverflowError empty struct, but there is no precedent for such a type.

I’ve used the to_* naming convention because this feels more of a conversion than as_secs which just exposes a value as it is stored internally, but this is a very weak opinion.

While we’re at it, and since from_secs and from_millis already exist, this might be the occasion to also add:

  • from_micros
  • from_nanos (It would be the same as Duration::new(0, n)… but then again we do have from_secs which is the same as Duration::new(s, 0).)
  • to_millis
  • to_micros

@alexcrichton, what do you think?

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Stebalien
Copy link
Contributor

Alternatively, we can wait for rust-lang/rfcs#1504 to be implemented and return a u128.

@alexcrichton
Copy link
Member

I personally don't have many opinions here, but I believe @wycats and @sfackler probably might.

@nagisa
Copy link
Member

nagisa commented Aug 21, 2016

There’s also an RFC for Duration::as_millis which got closed (though only because the RFC really proposed subsec_millis).

I’m kinda against adding any more stuff to Duration before we get u128s.

@arthurprs
Copy link
Contributor

We should have this, but I'm afraid I agree with others to wait for 128bit datatypes. Option isn't exactly pretty, as most people would just be unwraping everywhere.

@brson brson added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 29, 2016
@brson
Copy link
Contributor

brson commented Aug 29, 2016

Suspect we'll wait for u128 but maybe libs team should discuss.

@alexcrichton
Copy link
Member

Discussed at libs triage today our conclusion was that there's enough possibilities here in flight that we'd prefer to have an RFC about this before moving forward with a PR. To be clear, the naming, presence of the methods, and arguments (none) all seem ok, it's purely just the return type. We're also very interested in indeed having these types! It just seemed that there were enough possibilities that an RFC is warranted to talk through them.

@alexcrichton
Copy link
Member

@SimonSapin would you be interested in opening an RFC?

@SimonSapin
Copy link
Contributor Author

Sorry, shepherding an RFC sounds like more time (!) than I want to put into this right now. I imagine that RFC would quickly expand to consider using u128 in more places such as the internal representation of Duration.

@SimonSapin SimonSapin deleted the duration_as_nanos branch September 14, 2016 07:42
@arthurprs
Copy link
Contributor

arthurprs commented Sep 14, 2016

Maybe we can add to_millis -> u64 and call it?

I'm assuming that since we have from_millis that would be a no brainer and it's probably the most useful variant anyway.

@SimonSapin
Copy link
Contributor Author

@arthurprs Duration can represent more milliseconds than u64::MAX. Should to_millis -> u64 panic on overflow?

@arthurprs
Copy link
Contributor

Yes, but that's the tricky decision. In practice it'll only fail due to bogus input.

@arthurprs
Copy link
Contributor

arthurprs commented Sep 14, 2016

Another option is to make and document it as saturating, effectively returning u64::MAX on overflow. In humane terms that's 584,942,417 years.

For the sake of efficiency I checked some assembly outputs, https://godbolt.org/g/y0H9u2 . In summary it's a couple of conditional moves in addition to the arithmetic ops.

If we go the u128 route we're going to have a lot of as 64 around which is unchecked in release mode anyway.

@alexcrichton
Copy link
Member

@SimonSapin ah ok, no worries!

@arthurprs the problem here is that we have a number of possible return values:

  • Return u64 and panic on overflow
  • Return u64 and saturate on overflow
  • Return u128
  • Return Option<u64>

Each of these has various pros/cons, and this is what the libs team was thinking we'd need an RFC for (to weigh all of the decisions against one another)

@arthurprs
Copy link
Contributor

@alexcrichton Makes sense. I'll try to write something if someone doesn't beat me to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

7 participants