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.

Sign up for GitHub

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

Jump to bottom

Makes the constructors of Duration const fns. #47300

Merged
merged 3 commits into from
Jan 24, 2018
Merged

Makes the constructors of Duration const fns. #47300

merged 3 commits into from
Jan 24, 2018

Conversation

remexre
Copy link
Contributor

@remexre remexre commented Jan 9, 2018

This affects Duration::new, Duration::from_secs, Duration::from_millis, Duration::from_micros, and Duration::from_nanos.

@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 @aidanhs (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.

@remexre remexre mentioned this pull request Jan 9, 2018
Closed
3 tasks
oli-obk
oli-obk reviewed Jan 9, 2018
View reviewed changes
src/libstd/time/duration.rs Outdated
@@ -73,7 +73,7 @@ impl Duration {
/// ```
#[stable(feature = "duration", since = "1.3.0")]
#[inline]
pub fn new(secs: u64, nanos: u32) -> Duration {
pub const fn new(secs: u64, nanos: u32) -> Duration {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think checked_add and expect are const fn yet and can't be made const fn yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, I'll remove new from the PR then.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 9, 2018

It's not possible to have local variables in const fn yet. You'd need to convert everything to expression syntax.

@remexre
Copy link
Contributor Author

remexre commented Jan 9, 2018

Yeah, I committed the sin of PR'ing before the tests finished running locally... Should be fixed now?

shepmaster
shepmaster reviewed Jan 9, 2018
View reviewed changes
src/libstd/time/duration.rs Outdated
pub const fn from_millis(millis: u64) -> Duration {
Duration {
secs: millis / MILLIS_PER_SEC,
nanos: ((millis % MILLIS_PER_SEC) as u32) * NANOS_PER_MILLI,
Copy link
Member

Choose a reason for hiding this comment

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

You seem to have added literal tabs instead of 4 spaces.

@kennytm kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2018
@Mark-Simulacrum
Copy link
Member

r? @sfackler

@Mark-Simulacrum Mark-Simulacrum assigned sfackler and unassigned aidanhs Jan 10, 2018
@sfackler
Copy link
Member

These seem reasonable to me to become const fns.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 10, 2018 •
edited by Kimundi
Loading

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 Jan 10, 2018
@alexcrichton alexcrichton mentioned this pull request Jan 16, 2018
Merged
@kennytm
Copy link
Member

kennytm commented Jan 17, 2018

Triage ping, ticky boxes for you! @BurntSushi @Kimundi

@kennytm kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2018
@rfcbot
Copy link

rfcbot commented Jan 23, 2018

🔔 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 Jan 23, 2018
@rfcbot
Copy link

rfcbot commented Jan 23, 2018

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

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 23, 2018

📌 Commit c25178c has been approved by alexcrichton

@kennytm kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 24, 2018
@kennytm kennytm removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 24, 2018
@bors
Copy link
Contributor

bors commented Jan 24, 2018

⌛ Testing commit c25178c with merge a0dcecf...

bors added a commit that referenced this pull request Jan 24, 2018
@bors
Auto merge of #47300 - remexre:duration-constructors-as-const-fns, r=…
a0dcecf 
…alexcrichton

Makes the constructors of Duration const fns.

This affects `Duration::new`, `Duration::from_secs`, `Duration::from_millis`, `Duration::from_micros`, and `Duration::from_nanos`.
@bors
Copy link
Contributor

bors commented Jan 24, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing a0dcecf to master...

@bors bors merged commit c25178c into rust-lang:master Jan 24, 2018
@WiSaGaN WiSaGaN mentioned this pull request Feb 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shepmaster shepmaster shepmaster left review comments

@oli-obk oli-obk oli-obk left review comments

Assignees

@sfackler sfackler

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.