Skip to content

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

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

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

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
Collaborator

rfcbot commented Sep 15, 2018

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
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.

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

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

@rfcbot
Copy link
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

bors commented Sep 29, 2018

⌛ Testing commit 95c1d81 with merge bb0896a...

bors added a commit that referenced this pull request Sep 29, 2018
@bors
Copy link
Collaborator

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
# for free to join this conversation on GitHub. Already have an account? # to comment
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
Development

Successfully merging this pull request may close these issues.

T should implement From<NonZeroT>
9 participants