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

Impl From<NonZero<T>> for T #54240

Merged
merged 4 commits into from
Sep 29, 2018
Merged

Impl From<NonZero<T>> for T #54240

merged 4 commits into from
Sep 29, 2018

Conversation

csmoe
Copy link
Member

@csmoe csmoe commented Sep 15, 2018 •
edited
Loading

Closes #54171

r? @SimonSapin

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2018
@kennytm kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. labels Sep 15, 2018
@SimonSapin
Copy link
Contributor

Looks good to me. Let’s get team sign off since impls are insta-stable:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Sep 15, 2018 •
edited by Kimundi
Loading

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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 proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 15, 2018
scottmcm
scottmcm reviewed Sep 17, 2018
View reviewed changes
src/test/run-pass/nonzero.rs Outdated
fn main() {
use std::num::NonZeroU32;
let nz = NonZeroU32::new(5).unwrap();
let num: u32 = nz.into();
Copy link
Member

Choose a reason for hiding this comment

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

nit: perhaps just make this a doctest on the impl, so users can see it?

Copy link
Member Author

Choose a reason for hiding this comment

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

most of the types like String/PathBuf in std doesn't have specific docs for their From implementation, and invoking .from/.into is kind of intuition, so I think that's fine without doctest? but if it's really needed, I'd add it.

Copy link
Member

Choose a reason for hiding this comment

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

Definitely not a need. I just found a run-pass test odd for this, as x.py test src/libcore doesn't run those. src/libcore/tests/nonzero.rs might also be a reasonable place for the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

src/libcore/tests/nonzero.rs also makes sense to me.

src/libcore/num/mod.rs Outdated
@@ -93,6 +93,13 @@ assert_eq!(size_of::<Option<std::num::", stringify!($Ty), ">>(), size_of::<", st

}

#[stable(feature = "nonzero", since = "1.28.0")]
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think it'll be 1.31 now?

@rust-highfive

This comment has been minimized.

Sign in to view
@SimonSapin
Copy link
Contributor

The CI failure means this should use a new feature name, something like from_nonzero.

@rust-highfive

This comment has been minimized.

Sign in to view
@rfcbot
Copy link

rfcbot commented Sep 19, 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 Sep 19, 2018
@rfcbot
Copy link

rfcbot commented Sep 29, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 29, 2018
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 29, 2018

📌 Commit 95c1d81 has been approved by alexcrichton

@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 Sep 29, 2018
@bors
Copy link
Contributor

bors commented Sep 29, 2018

⌛ Testing commit 95c1d81 with merge bb0896a...

bors added a commit that referenced this pull request Sep 29, 2018
@bors
Auto merge of #54240 - csmoe:nonzero_from, r=alexcrichton
bb0896a 
Impl From<NonZero<T>> for T

Closes #54171

r? @SimonSapin
@bors
Copy link
Contributor

bors commented Sep 29, 2018

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

@bors bors merged commit 95c1d81 into rust-lang:master Sep 29, 2018
@csmoe csmoe deleted the nonzero_from branch September 29, 2018 23:33
@Centril Centril added this to the 1.31 milestone Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonSapin SimonSapin SimonSapin left review comments

@scottmcm scottmcm scottmcm left review comments

Assignees

@SimonSapin SimonSapin

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. 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
1.31
Development

Successfully merging this pull request may close these issues.

T should implement From<NonZeroT>
9 participants
@csmoe @SimonSapin @rfcbot @rust-highfive @alexcrichton @bors @scottmcm @kennytm @Centril