Specify NonZero and Option<NonZero> layout
#104082
Conversation
|
r? @scottmcm (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
@rustbot label +T-libs-api -T-libs |
rustbot
added
T-libs-api
library/core/src/num/nonzero.rs
Outdated
| /// | ||
| #[doc = concat!("`", stringify!($Ty), "` has the same layout as `", stringify!($Int), "`, except that zero is not")] | ||
| #[doc = concat!("a valid instance of `", stringify!($ty), "`. `Option<", stringify!($Ty), ">` has the same layout")] | ||
| #[doc = concat!("and ABI as `", stringify!($Int), "`.")] |
There was a problem hiding this comment.
Is this ABI a guarantee that's documented somewhere else right now? Or is it a new promise?
Maybe this could start by just changing the "size" to "size and alignment" in the existing paragraph and code. That much, at least, is very non-controversial promise AFAIK.
@rustbot author
There was a problem hiding this comment.
Given that it's not yet well-defined, I'm using "ABI" here to mean layout and bit validity. How would you feel if I just said "layout and bit validity" explicitly here? As you said, layout is a non-controversial promise. Bit validity is, in turn, a logical implication.
Specifically, an n-bit non-zero type can represent 2^n - 1 states, and so Option<NonZeroXxx> can represent 2^n states (the None variant adds one more possible state). This is the same number of states as exist in the corresponding n-bit primitive type. The layout (and thus the size) of Option<NonZeroXxx> is guaranteed to be the same as that of the corresponding primitive - namely n bits. Since there are 2^n possible states and n bits used to represent them, every one of the 2^n possible n-bit sequences must correspond to a valid state. The same is true by definition for the corresponding n-bit primitive. Thus, Option<NonZeroXxx> and the corresponding primitive have the same bit validity.
There was a problem hiding this comment.
@scottmcm Bump :) See also #94786 (comment).
There was a problem hiding this comment.
Oh, sorry for taking a million years here -- it was still waiting-on-author and thus not in my queue.
How about leaving the header as layout, but explicitly saying "size", "alignment", and "bit pattern" in the prose? When the list isn't long, might as well just be extra-clear by saying all of them.
It looks like "size" is already used in https://doc.rust-lang.org/std/option/index.html#representation, for example.
There was a problem hiding this comment.
Actually, I just noticed that #94786 landed. How do you want to proceed with this given that one happened?
There was a problem hiding this comment.
I've rebased on that. The wording change here is now much simpler - just:
...is guaranteed to be compatible with
Xxx, including in FFI.
to:
...is guaranteed to have the same layout and bit validity as
Xxx.
My intention is that the latter language implies the former, so it shouldn't be problematic to remove the bit about FFI. I also haven't seen the phrase "compatible with" used elsewhere in documentation relating to layout or bit validity, so my hope is that the latter language is more clear if you're already familiar with other such docs.
cc @joshtriplett since IIUC, the "compatible with" language was your suggestion in the original PR.
rustbot
added
S-waiting-on-author
|
@joshlf FYI: when a PR is ready for review, send a message containing |
|
@JohnCSimon apologies for the delay in responding! I've followed up on the outstanding comment thread. |
JohnCSimon
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #94786) made this pull request unmergeable. Please resolve the merge conflicts. |
|
triage - |
|
@joshlf what's the status of this pr? |
|
Apologies! Waiting for a response on https://github.com/rust-lang/rust/pull/104082/files#r1178424303. |
|
☔ The latest upstream changes (presumably #115469) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Given the updates that have happened to https://doc.rust-lang.org/nightly/core/num/struct.NonZeroU16.html#layout-1 and https://doc.rust-lang.org/nightly/std/option/index.html#representation, I think this is now covered? Please re-activate (before force-pushing, because github needs that!) if I'm misunderstanding things here and you're still looking for a new guarantee here. |
No description provided.