Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add more Duration methods for consistency. #46508

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

clarfonthey
Copy link
Contributor

Follow-up to #46507.

@rust-highfive
Copy link
Collaborator

r? @KodrAus

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

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 5, 2017
@sfackler
Copy link
Member

sfackler commented Dec 5, 2017

I'm on board with these methods!

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 5, 2017

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 5, 2017
@sfackler
Copy link
Member

sfackler commented Dec 5, 2017

Tidy's failing

/// ```
#[unstable(feature = "duration_extras", issue = "46507")]
#[inline]
pub fn from_nanos(nanos: u64) -> Duration {
Copy link
Member

Choose a reason for hiding this comment

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

This feels like it should be from_nanosecs or something like that by the convention in other methods.

Copy link
Member

Choose a reason for hiding this comment

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

The existing constructors are new, from_secs, from_millis, and from_micros, so it seems like from_nanos would match conventions?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we've been somewhat inconsistent with abbreviating seconds already, so this seems fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally like the use of the pluralised prefices, considering how simply using ns, us, and ms would be very easy to make typos. I don't personally like using sec as an abbreviation for second considering how sec is secant and s is second, but that's just because I'm a maths person and I appreciate clarity in equations. We shouldn't really change the naming scheme now that this has been stabilised for a long time.

@kennytm kennytm added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 5, 2017
@retep998
Copy link
Member

retep998 commented Dec 5, 2017

I personally would rather that from_nanos take u128.

@kennytm
Copy link
Member

kennytm commented Dec 5, 2017

@retep998 A u128 nanoseconds will overflow Duration which only supports u64 seconds (2128 / 109 = 3.4 × 1029; 264 = 1.8 × 1019). If it accepts a u128, either the method needs to return Result<Duration, _> or it needs to panic on overflow, or we need to change the representation of Duration to use u128 nanoseconds internally.

@clarfonthey
Copy link
Contributor Author

@retep998 u64 of nanoseconds covers approximately 584 942 years. do we really need more precision?

@clarfonthey
Copy link
Contributor Author

also @sfackler the build is passing now

@retep998
Copy link
Member

retep998 commented Dec 5, 2017

@clarcharr You can ask that question to whoever designed Duration as u64 seconds and u32 nanoseconds instead of simply u64 nanoseconds.

@sfackler
Copy link
Member

sfackler commented Dec 5, 2017

@retep998 I don't see why that's a problem. If you can't initialize a Duration via a number of nanoseconds, you can do it via new. This is already the case for from_millis.

@retep998
Copy link
Member

retep998 commented Dec 5, 2017

Ah right, from_millis has the same tradeoff. In that case I'm fine with from_nanos as written.

@clarfonthey
Copy link
Contributor Author

Does this need the final check from @aturon considering how all of these methods are already feature-gated? Was going to make a PR for #46520 but I want to merge this first.

@kennytm
Copy link
Member

kennytm commented Dec 14, 2017

Ping @aturon for ticky boxes!

@rfcbot
Copy link

rfcbot commented Dec 19, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 19, 2017
@alexcrichton
Copy link
Member

@bors: r=sfackler

@bors
Copy link
Contributor

bors commented Dec 19, 2017

📌 Commit 2be7a31 has been approved by sfackler

@bors
Copy link
Contributor

bors commented Dec 20, 2017

⌛ Testing commit 2be7a31 with merge 16212b9...

bors added a commit that referenced this pull request Dec 20, 2017
Add more Duration methods for consistency.

Follow-up to #46507.
@kennytm kennytm 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 20, 2017
@bors
Copy link
Contributor

bors commented Dec 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 16212b9 to master...

@bors bors merged commit 2be7a31 into rust-lang:master Dec 20, 2017
bors added a commit that referenced this pull request Jan 31, 2018
Move Duration to libcore

Fixes #46520; should be merged after #46508.
@clarfonthey clarfonthey deleted the duration_extras branch April 15, 2018 20:31
@retep998
Copy link
Member

@retep998 u64 of nanoseconds covers approximately 584 942 years. do we really need more precision?

@clarcharr Did you mean microseconds, because u64 of nanoseconds only goes to 585 years which is actually quite short.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. 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.

10 participants