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

Clarify documentation about the memory layout of UnsafeCell #103394

Merged
merged 3 commits into from
Oct 28, 2022
Merged

Clarify documentation about the memory layout of UnsafeCell #103394

merged 3 commits into from
Oct 28, 2022

Conversation

Pointerbender
Copy link
Contributor

This PR addresses a comment by @RalfJung in PR #101717 to further clarify the documentation of UnsafeCell<T>. The previous PR was merged already before we had a chance to correct this, hence this second PR :)

To goal of this PR is:

  1. Split the paragraph about the memory layout of UnsafeCell<T> and the usage of UnsafeCell::(raw_)get() into two paragraphs, so that it is easier to digest for the reader.
  2. Slightly simplify the previously added examples in order to reduce redundancy between the new examples and the examples that already existed before these 2 PRs (which remained untouched by both PRs).

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 22, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 22, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Oct 22, 2022
RalfJung
RalfJung reviewed Oct 22, 2022
View reviewed changes
library/core/src/cell.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Oct 22, 2022
View reviewed changes
library/core/src/cell.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Nice examples, thanks! I just left one minor edit. My other concern is that this is hidden so late in the documentation that I am a bit worried people might miss it. Are there some reasonable subsections we could use to structure this?

r? @Amanieu since this is a follow-up to #101717

@rust-highfive rust-highfive assigned Amanieu and unassigned m-ou-se Oct 22, 2022
@Pointerbender @RalfJung
fix typos
5673536 
Co-authored-by: Ralf Jung <post@ralfj.de>
@Pointerbender
Copy link
Contributor Author

Are there some reasonable subsections we could use to structure this?

The new text that was added in this PR and in #101717 cover the topics of memory layout and type conversion. As the text is currently written down, I think we can combine this into one subsection labeled "Memory layout and type conversions", but an alternative could be to detangle the text even further and split it into two subsections "Memory layout" and "Type conversions". As the text is structured now, the only sentence that seems relevant to said memory layout subsection is "UnsafeCell<T> has the same in-memory representation as its inner type T.", the rest of the text explains about the pitfalls when converting between UnsafeCell<T> and T (namely: disabled niches, use .get() instead of casting). Therefore I think just adding a "Memory layout and type conversions" header feels the most natural, but please let me know what you prefer @RalfJung :)

@RalfJung
Copy link
Member

RalfJung commented Oct 25, 2022 •
edited
Loading

"Memory layout" sounds good, but "type conversion" sounds like a safe operation, like as or into.
Sadly we don;t have a standard term for 'reinterpretation' or 'type punning' or 'transmutation'.

Maybe just "memory layout"?

@Pointerbender
Copy link
Contributor Author

Maybe just "memory layout"?

Sounds good to me, I just updated the PR. Thanks!

@RalfJung
Copy link
Member

LGTM, let's see what @Amanieu thinks.

@Amanieu
Copy link
Member

Amanieu commented Oct 27, 2022

LGTM!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 27, 2022

📌 Commit 166d8b8 has been approved by Amanieu

It is now in the queue for this repository.

@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 Oct 27, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 27, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2022
@bors
Auto merge of rust-lang#103629 - matthiaskrgr:rollup-r94tqfa, r=matth…
126dbdc 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#103110 (remove redundant Send impl for references)
 - rust-lang#103255 (Clean up hidden type registration)
 - rust-lang#103394 (Clarify documentation about the memory layout of `UnsafeCell`)
 - rust-lang#103408 (Clean return-position `impl Trait` in traits correctly in rustdoc)
 - rust-lang#103505 (rustdoc: parse self-closing tags and attributes in `invalid_html_tags`)
 - rust-lang#103524 (rustc_metadata: Add struct and variant constructors to module children at encoding time)
 - rust-lang#103544 (Add flag to forbid recovery in the parser)
 - rust-lang#103616 (rustdoc: remove CSS workaround for Firefox 29)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c75e6f5 into rust-lang:master Oct 28, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@Amanieu Amanieu

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Pointerbender @rustbot @rust-highfive @RalfJung @Amanieu @bors @m-ou-se