Add Duration from nanos u128 by omanirudh · Pull Request #139243 · 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

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 }})

What does this PR do?

This draft PR adds the Duration::from_nanos_u128 function to handle durations that exceed the range of u64, allowing for time spans greater than ~584 years.

Motivation

The current Duration API does not support creating durations from nanoseconds represented as u128. This addition addresses that limitation.

Tracking Issue

Fixes #139201

Details

Introduced Duration::from_nanos_u128 as a const fn similar to other functions in the file.
Ensured safety by validating the nanoseconds before using unsafe code.
To do : complete the documentation and examples for the new function.
r? @RalfJung

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Apr 2, 2025

This comment has been minimized.

Thanks for taking this over!
However, this needs a libs reviewer. I just proposed the API. :)

r? libs

Thanks for taking this over! However, this needs a libs reviewer. I just proposed the API. :)

r? libs

Happy to take this up. noted.

rustbot 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

Apr 2, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

This comment has been minimized.

omanirudh marked this pull request as ready for review

April 10, 2025 15:53

rustbot 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

Apr 11, 2025

#[inline]
pub const fn from_nanos_u128(nanos: u128) -> Duration {
const NANOS_PER_SEC: u128 = self::NANOS_PER_SEC as u128;
let secs: u64 = (nanos / NANOS_PER_SEC) as u64;

Choose a reason for hiding this comment

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

The function documentation currently states that the function panics on overflow,
but this cast truncates the result of the division instead of panicking on overflow.

e.g. u128::MAX results in 13088917067439035463.768211455s instead of panicking.

playground

Choose a reason for hiding this comment

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

thanks for pointing it out @Skgland
will add the following check and ensure it panics on overflow

if expected_secs > u64::MAX.into() { panic!("overflow in duration in Duration::from_nanos_u128"); }

This comment has been minimized.

panic!("overflow in duration in Duration::from_nanos_u128");
}
let subsec_nanos = (nanos % NANOS_PER_SEC) as u32;
// SAFETY: x % 1_000_000_000 < 1_000_000_000

Choose a reason for hiding this comment

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

new_unchecked is unsafe since there is no bounds check being performed on the input value. Here is the reference.

(The comment saying that val == 0 is language UB doesn't exactly apply here since the Nanosecond type is defined to be between 0 and 999_999_999 both inclusive).

Just so that our safety assumption is clear here, do add the fact that subsec_nanos >= 0 because u128 >=0 and u32 >= 0

Rest of the code looks good to me, amazing work!

Only need to make small changes on the documentation side of things (along with @Skgland 's review about panicking on overflow instead of truncating on overflow).

Do await a review from someone from the libs team too.

About the commit history (this is completely upto you)

You can use git rebase -i HEAD~<number of commits>. This opens up an interactive window using which you can reorganize and meld related commits together.

This is a related thread on zulip on the concerns behind squashing vs merging commits.

rustbot 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

Apr 15, 2025

This comment has been minimized.

Add unstable attribute

Add unstable attribute for from_nanos_u128 with tracking issue rust-lang#139201

Co-authored-by: Madhav Madhusoodanan madhavmadhusoodanan@gmail.com

add feature gate for the new function

add feature gate
remove trailing whitespace

Add feature gate at the right place

Co-authored-by: Madhav Madhusoodanan madhavmadhusoodanan@gmail.com

Update time.rs

making sure seconds are in u64

Co-authored-by: Madhav Madhusoodanan madhavmadhusoodanan@gmail.com

Update time.rs

keeping seconds as u64 removing the use of non const function within const function

Use a u128 example time.rs

Earlier for the example I used 2.pow(64). correcting it to use u128 type which can contain the value

Synched my fork Dummy commit

dummy commit.

sync local with fork

style: format code with rustfmt

Add panic test for Duration::from_nanos_u128

fix: remove non-const .into() call from const function A const function cannot call non-const functions, so we remove the trailing .into() call and use raw literals instead.

remove paratheses

Add comment on largest input and explain safety assumption

Ran Rust fmt again

rustbot 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

Apr 21, 2025

About the commit history (this is completely upto you)

You can use git rebase -i HEAD~<number of commits>. This opens up an interactive window using which you can reorganize and meld related commits together.

This is a related thread on zulip on the concerns behind squashing vs merging commits.

I looked into it. This PR now has just one commit. I preserved the commit messages of earlier commits though.

Choose a reason for hiding this comment

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

Implementation and docs look good to me, left a few small comments about adding backticks around code types. However, this needs some tests. Could you update library/coretests/tests/time.rs to include from_nanos_u128 everywhere that from_nanos is tested? Also make sure some large values are tested, and one or two #[should_panic] tests for the overflow.

@@ -308,6 +307,39 @@ impl Duration {
Duration { secs, nanos: subsec_nanos }
}

/// Creates a new Duration from the specified number of nanoseconds.

Choose a reason for hiding this comment

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

/// Creates a new Duration from the specified number of nanoseconds.
/// Creates a new `Duration` from the specified number of nanoseconds.

///
/// # Panics
///
/// Panics if the given number of nanoseconds is greater than what Duration can handle,

Choose a reason for hiding this comment

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

/// Panics if the given number of nanoseconds is greater than what Duration can handle,
/// Panics if the given number of nanoseconds is greater than what `Duration` can handle,

///
/// Panics if the given number of nanoseconds is greater than what Duration can handle,
/// which is `(u64::MAX * NANOS_PER_SEC) + NANOS_PER_SEC - 1`
/// Use this function if you need to specify time greater than what can fit in u64

Choose a reason for hiding this comment

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

/// Use this function if you need to specify time greater than what can fit in u64
/// Use this function if you need to specify time greater than what can fit in `u64`

@@ -1,5 +1,4 @@
#![stable(feature = "duration_core", since = "1.25.0")]

Choose a reason for hiding this comment

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

Looks like a spurious removal

rustbot 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

Apr 26, 2025

I looked into it. This PR now has just one commit. I preserved the commit messages of earlier commits though.

Also fyi - pedantically, there is no need to preserve the messages unless they are still meaningful. It's best if the message describes the contained change, it's not really useful to rust's forever history that you had "dummy commit" and "remove parentheses" at some point :)

I looked into it. This PR now has just one commit. I preserved the commit messages of earlier commits though.

Also fyi - pedantically, there is no need to preserve the messages unless they are still meaningful. It's best if the message describes the contained change, it's not really useful to rust's forever history that you had "dummy commit" and "remove parentheses" at some point :)

noted.

Labels

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.