-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Tarpl clarity #27421
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.
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
Tarpl clarity #27421
Conversation
Saying it stores a discriminant bit inside the pointer is wrong, for the notion of inside that most people will assume (e.g.: which bit of the pointer is used?). And I may be totally totally wrong here but I was under the impression that it's a very narrow optimization that doesn't apply to anything beyond a pointer + a unit. Saying so explicitly seems much clearer to me and prevents wild imaginings about enums with 4 pointer variants and a bool variant or whatever.
The prior example was explicitly *not* contiguous (e.g. the values all next to adjacent in memory), so that's confusing way of putting it.
Reduces the number of things going on to focus on what matters, which is presumably that the two identical structs A and B aren't guarenteed to have the same layout.
If you read this sentence carefully, the last phrase doesn't make sense, because "not being guarenteed" is not a reason why they explicitly wouldn't. Unless the goal of the compiler is to be maximally confusing. Also, this example isn't nonsensical, it's just pedantic (because the layouts of A and B *are* the same, though they're not guarenteed to be), but it leads to this more important point about layout flexibility. Saying nonsensical makes it sound like the text itself is nonsensical.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gankro (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. 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. |
|
||
However there are several cases where such a representation is inefficient. The | ||
classic case of this is Rust's "null pointer optimization": an enum consisting | ||
of a unit variant and a non-nullable pointer variant (e.g. `&u32`) makes the tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite strong enough -- Rust can dig deep into types to find that pointer. For instance Option<Vec<T>>
also optimizes. However to my knowledge it can't dig through other enums or use multiple nullable pointers -- so Option<Option<(&u8, &u8)>>
is 24-bytes on 64 bit (even though it could be 16).
Also "unit variant" strikes me as a bit jargony, though I have no great concrete proposal -- perhaps introduce the notion of a C-like variant?
I fear this will conflict with #27414 as I cleaned up some italics here. There's not a lot that can be done, though. One of us will win the queue race and have to rebase (by default my PR will get priority because the system favours older PRs). Still this is great! (although 7 commits is a bit gratuitous) |
No worries will rebase if necessary tomorrow. I can do fewer commits next time also. My habit is to make them totally atomic changes using interactive staging. |
☔ The latest upstream changes (presumably #27414) made this pull request unmergeable. Please resolve the merge conflicts. |
- Successful merges: rust-lang#27464, rust-lang#27473 - Failed merges:
Saying it stores a discriminant bit inside the pointer is wrong, for the notion of inside that most people will assume (e.g.: which bit of the pointer is used?). And I may be totally totally wrong here but I was under the impression that it's a very narrow optimization that doesn't apply to anything beyond a pointer + a unit. Saying so explicitly seems much clearer to me and prevents wild imaginings about enums with 4 pointer variants and a bool variant or whatever.
The prior example was explicitly *not* contiguous (e.g. the values all next to adjacent in memory), so that's confusing way of putting it.
Reduces the number of things going on to focus on what matters, which is presumably that the two identical structs A and B aren't guarenteed to have the same layout.
If you read this sentence carefully, the last phrase doesn't make sense, because "not being guarenteed" is not a reason why they explicitly wouldn't. Unless the goal of the compiler is to be maximally confusing. Also, this example isn't nonsensical, it's just pedantic (because the layouts of A and B *are* the same, though they're not guarenteed to be), but it leads to this more important point about layout flexibility. Saying nonsensical makes it sound like the text itself is nonsensical.
@gankro I've tried to clarify the null pointer optimization thing. Think it's accurate now? Also, giving an example for "unit variant" should clarify it for people who've forgotten, but if the assumption is that they've read trpl first it seems to me fair to assume they've at least encountered the idea before. What do you think? edit: also I rebased, hopefully I didn't muck anything up. |
classic case of this is Rust's "null pointer optimization": an enum consisting | ||
of a single outer unit variant (e.g. `None`) and a (potentially nested) non- | ||
nullable pointer variant (e.g. `&T`) makes the tag unnecessary, because a null | ||
pointer value can safely be interpreted tos mean that the unit variant is chosen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/tos/to
Thanks! Just one nit. It seems something went wacky in your rebase, though (possibly because you did a merge and not a pure rebase..?). r=me once you've sorted that out and squashed your commits down. |
☔ The latest upstream changes (presumably #27444) made this pull request unmergeable. Please resolve the merge conflicts. |
@taliesinb :S now the renaming landed (it's at src/doc/nomicon now). A |
@gankro I've created a new PR because this history as you say is all messed up. Closing this one... |
Bunch of rephrasings and perhaps one technical thing about null pointer optimizations (about which I may be wrong). I understand if much of this is subjective, but I really do feel that these edits are all clarity-improving (I write a fair amount of technical documentation during my day job).
Individual commits have justifications/explanations as commit messages.
r? @gankro