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

Make the size of Option<NonZero*> a documented guarantee. #51396

Merged
merged 1 commit into from
Jun 9, 2018
Merged

Make the size of Option<NonZero*> a documented guarantee. #51396

merged 1 commit into from
Jun 9, 2018

Conversation

SimonSapin
Copy link
Contributor

Closes #49137, the tracking issue for NonZero*, as this was the last remaining open question.

Note that ptr::NonNull<T> already documents a similar guarantee.

@SimonSapin SimonSapin added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jun 6, 2018
@rust-highfive
Copy link
Collaborator

r? @KodrAus

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2018
@SimonSapin
Copy link
Contributor Author

SimonSapin commented Jun 6, 2018 •
edited
Loading

I’d left this out explicitly out of rust-lang/rfcs#2307 in order to have one fewer question to resolve at RFC time, but given the precedent of Option<&T> and Option<NonNull<T>> and others I think it’s reasonable to do this.

Lang team, this is your decision:

@rfcbot fcp merge

Edit: uh, looks like one needs to be part of the team to start FCP.

@SimonSapin
Make the size of Option<NonZero*> a documented guarantee.
5c7ca77 
Closes rust-lang#49137, the tracking issue for `NonZero*`,
as this was the last remaining open question.

Note that `ptr::NonNull<T>` already documents a similar guarantee.
@SimonSapin SimonSapin force-pushed the option-nonzero-layout branch from 65bc71f to 5c7ca77 Compare June 6, 2018 11:58
@SimonSapin SimonSapin changed the title Make the size of Option<NonZero*> a document guarantee. Make the size of Option<NonZero*> a documented guarantee. Jun 6, 2018
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

This PR documents the assumption that Option<NonZeroU32> will be the size of a u32. The whole point of the non-zero types is to enable this optimization. We should guarantee it.

@rfcbot
Copy link

rfcbot commented Jun 6, 2018 •
edited by nrc
Loading

Team member @nikomatsakis 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 Jun 6, 2018
@rfcbot
Copy link

rfcbot commented Jun 7, 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 Jun 7, 2018
@glandium
Copy link
Contributor

glandium commented Jun 8, 2018

Should the same guarantee, that applies to e.g. Box, Rc, Arc, Vec, String, ... be documented for those types as well? (There's a mention for Box in the documentation of size_of).

@SimonSapin
Copy link
Contributor Author

@glandium Maybe, but I made this PR specifically in the context of closing #49137, the tracking issue for NonZero* types.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2018

📌 Commit 5c7ca77 has been approved by SimonSapin

@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 Jun 8, 2018
@kennytm
Copy link
Member

kennytm commented Jun 8, 2018

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 8, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#51396 - SimonSapin:option-nonzero-layout, r…
b1a6db2 
…=SimonSapin

Make the size of Option<NonZero*> a documented guarantee.

Closes rust-lang#49137, the tracking issue for `NonZero*`, as this was the last remaining open question.

Note that `ptr::NonNull<T>` already documents a similar guarantee.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jun 8, 2018
Merged
bors added a commit that referenced this pull request Jun 8, 2018
@bors
Auto merge of #51448 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
cf91e9b 
Rollup of 13 pull requests

Successful merges:

 - #50143 (Add deprecation lint for duplicated `macro_export`s)
 - #51099 (Fix Issue 38777)
 - #51276 (Dedup auto traits in trait objects.)
 - #51298 (Stabilize unit tests with non-`()` return type)
 - #51360 (Suggest parentheses when a struct literal needs them)
 - #51391 (Use spans pointing at the inside of a rustdoc attribute)
 - #51394 (Use scope tree depths to speed up `nearest_common_ancestor`.)
 - #51396 (Make the size of Option<NonZero*> a documented guarantee.)
 - #51401 (Warn on `repr` without hints)
 - #51412 (Avoid useless Vec clones in pending_obligations().)
 - #51427 (compiletest: autoremove duplicate .nll.* files (#51204))
 - #51436 (Do not require stage 2 compiler for rustdoc)
 - #51437 (rustbuild: generate full list of dependencies for metadata)

Failed merges:
@bors bors merged commit 5c7ca77 into rust-lang:master Jun 9, 2018
@SimonSapin SimonSapin deleted the option-nonzero-layout branch June 9, 2018 22:02
@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 Jun 17, 2018
@scottmcm scottmcm mentioned this pull request Mar 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@KodrAus KodrAus

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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language 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.
8 participants
@SimonSapin @rust-highfive @nikomatsakis @rfcbot @glandium @bors @kennytm @KodrAus