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 as_nanos function to Duration #50167

Merged
merged 7 commits into from
Jun 3, 2018
Merged

Conversation

fintelia
Copy link
Contributor

Duration has historically lacked a way to get the actual number of nanoseconds it contained as a normal Rust type because u64 was of insufficient range, and f64 of insufficient precision. The u128 type solves both issues, so I propose adding an as_nanos function to expose the capability.

@rust-highfive
Copy link
Collaborator

r? @aidanhs

(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 Apr 22, 2018
@rust-highfive

This comment has been minimized.

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 24, 2018
/// let duration = Duration::new(5, 730023852);
/// assert_eq!(duration.as_nanos(), 5730023852);
/// ```
#[unstable(feature = "duration_nanos", issue = "50167")]
Copy link
Member

Choose a reason for hiding this comment

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

The tracking issue number should be a new issue, not this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@pietroalbini
Copy link
Member

Reassigning this to someone from the libs team.
r? @sfackler

@rust-highfive rust-highfive assigned sfackler and unassigned aidanhs Apr 30, 2018
@sfackler
Copy link
Member

sfackler commented May 1, 2018

We should add similar accessors for milliseconds and microseconds at the same time for consistency with the subsec methods.

cc @rust-lang/libs

@fintelia
Copy link
Contributor Author

fintelia commented May 1, 2018

@sfackler I can make that change. Any thoughts on what the feature should be called then? "duration_as_nanos" isn't really a great name if it also adds as_millis() and as_micros(). Perhaps "duration_as_u128"?

@sfackler
Copy link
Member

sfackler commented May 2, 2018

Sure, seems reasonable.

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2018
@pietroalbini
Copy link
Member

Ping from triage @fintelia! It's been a while since we heard from you, will you have time to work on this again soon?

@fintelia
Copy link
Contributor Author

Sorry about that. I'm probably going to be busy for about a week longer, so if someone else wants to take over making those changes, feel free

@pietroalbini
Copy link
Member

@fintelia don't worry, we just want to make sure you still remember about the PR ;)
Someone from triage will ping you next week.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 20, 2018
#[unstable(feature = "duration_as_u128", issue = "50202")]
#[inline]
pub fn as_millis(&self) -> u128 {
self.secs as u128 * MILLIS_PER_SEC as u128 + self.nanos as u128 / NANOS_PER_MILLI as u128
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: self.nanos as u128 / NANOS_PER_MILLI as u128 should be replaced with (self.nanos / NANOS_PER_MILLI) as u128. The former requires doing 128-bit arithmetic on a 32-bit number, which is redundant.

#[unstable(feature = "duration_as_u128", issue = "50202")]
#[inline]
pub fn as_micros(&self) -> u128 {
self.secs as u128 * MICROS_PER_SEC as u128 + self.nanos as u128 / NANOS_PER_MICRO as u128
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment from as_nanos

#[unstable(feature = "duration_as_u128", issue = "50202")]
#[inline]
pub fn as_nanos(&self) -> u128 {
self.secs as u128 * NANOS_PER_SEC as u128 + self.nanos as u128
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment from as_nanos

@shepmaster
Copy link
Member

Ping from triage, @sfackler !

@sfackler
Copy link
Member

sfackler commented Jun 2, 2018

Oops - seems good to me!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2018

📌 Commit fc89566 has been approved by sfackler

@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 2, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 2, 2018
 Add as_nanos function to Duration

Duration has historically lacked a way to get the actual number of nanoseconds it contained as a normal Rust type because u64 was of insufficient range, and f64 of insufficient precision. The u128 type solves both issues, so I propose adding an `as_nanos` function to expose the capability.
bors added a commit that referenced this pull request Jun 2, 2018
Rollup of 6 pull requests

Successful merges:

 - #50167 ( Add as_nanos function to Duration)
 - #50919 (Provide more context for what the {f32,f64}::EPSILON values represent.)
 - #51124 (Reword {ptr,mem}::replace docs.)
 - #51147 (Stabilize SliceIndex trait.)
 - #51291 (Fix typos of ‘ambiguous’)
 - #51302 (Permit building rustdoc without compiler artifacts)

Failed merges:
@bors bors merged commit fc89566 into rust-lang:master Jun 3, 2018
@bors
Copy link
Contributor

bors commented Jun 3, 2018

⌛ Testing commit fc89566 with merge 3515dab...

@bors
Copy link
Contributor

bors commented Jun 3, 2018

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

9 participants