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

Expand CloneToUninit documentation. #133055

Merged
merged 3 commits into from
Mar 16, 2025
Merged

Expand CloneToUninit documentation. #133055

merged 3 commits into from
Mar 16, 2025
+145 −30

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Nov 14, 2024

  • Clarify relationship to dyn after Make CloneToUninit dyn-compatible #133003.
  • Add an example of using it with dyn as Make CloneToUninit dyn-compatible #133003 enabled.
  • Replace parameter name dst with dest to avoid confusion between abbreviations for “DeSTination” and “Dynamically-Sized Type”.
  • Add an example of implementing it.
  • Add links to Rust Reference for the mentioned concepts.
  • Mention that its method should rarely be called.
  • Various small corrections.

Please review the unsafe code closely, as I am not an expert in the best possible ways to express these operations. (It might also be better to omit the implementation example entirely.)

cc @zachs18 #126799

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 14, 2024
@kpreid kpreid changed the title Clone uninit doc Expand CloneToUninit documentation. Nov 14, 2024
zachs18
zachs18 approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@zachs18 zachs18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unsafe code in the impl CloneToUninit for MyDst<T> example looks correct to me, with one note about a comment.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 7, 2025

☔ The latest upstream changes (presumably #138155) made this pull request unmergeable. Please resolve the merge conflicts.

@kpreid
Expand CloneToUninit documentation.
769425a 
* Clarify relationship to `dyn` after rust-lang#133003.
* Add an example of using it with `dyn` as rust-lang#133003 enabled.
* Add an example of implementing it.
* Add links to Rust Reference for the mentioned concepts.
* Mention that its method should rarely be called.
* Replace parameter name `dst` with `dest` to avoids confusion between
  “DeSTination” and “Dynamically-Sized Type”.
* Various small corrections.
@kpreid kpreid force-pushed the clone-uninit-doc branch from 64ec867 to 769425a Compare March 8, 2025 03:49
@kpreid
Copy link
Contributor Author

kpreid commented Mar 8, 2025

Rerolling due to lack of response.

r? libs

@rustbot rustbot assigned Mark-Simulacrum and unassigned cuviper Mar 8, 2025
Mark-Simulacrum
Mark-Simulacrum reviewed Mar 9, 2025
View reviewed changes
library/core/src/clone.rs Outdated
///
/// // Since `flag` implements `Copy`, we can just copy it.
/// // We use `pointer::write()` instead of assignment because the destination must be
/// // assumed to be uninitialized, whereas an assignment assumes it is initialized.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think assignment assumes initialized memory for Copy types, and Miri is fine with it: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=2d5d4affa1262eff4a7c5d1a2a7fd14b

The unsound operation is dropping the memory behind the pointer, but for Copy types that doesn't happen.

I'd probably still write it this way, but I don't think we should write something misleading in the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to write a good comment, and was unhappy with all the versions I came up with without also changing the code. I ended up changing the code to .clone_to_uninit() the flag too, because it's easier to justify as “most general”. Not sure that makes sense.

— actually, it doesn't make sense, because the easiest way to ensure the desirable no-memory-leaks property would be to strictly use .clone() on all the sized fields first (so the values are held in drop-tracked locals), then clone the uninit field. And then I don't have to hedge anything about leaking. Please wait while I rewrite again…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now rewritten the example so that

  • the sized field is not Copy, and
  • neither field is leaked on panic,

and there is one straightforward correct way to do that, so we no longer have any reason to talk about the other options. What do you think?

kpreid added a commit to kpreid/rust that referenced this pull request Mar 10, 2025
@kpreid
Expand and organize offset_of! documentation.
58d4395 
* Give example of how to get the offset of an unsized tail field
  (prompted by discussion <rust-lang#133055 (comment)>).
* Specify the return type.
* Add section headings.
* Reduce “Visibility is respected…”, to a single sentence.
@kpreid kpreid mentioned this pull request Mar 10, 2025
Merged
@Mark-Simulacrum
Copy link
Member

@bors r+

I think this is good.

@bors
Copy link
Contributor

bors commented Mar 15, 2025

📌 Commit 96814ae has been approved by Mark-Simulacrum

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 Mar 15, 2025
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 16, 2025
@jieyouxu
Rollup merge of rust-lang#138323 - kpreid:offset-of-doc, r=Mark-Simul…
5b92250 
…acrum

Expand and organize `offset_of!` documentation.

* Give example of how to get the offset of an unsized tail field (prompted by discussion <rust-lang#133055 (comment)>).
* Specify the return type.
* Add section headings.
* Reduce “Visibility is respected…”, to a single sentence.
* Move `offset_of_enum` documentation to unstable book (with link to it).
* Add `offset_of_slice` documentation in unstable book.

r? Mark-Simulacrum
@jieyouxu jieyouxu mentioned this pull request Mar 16, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2025
@bors
Auto merge of rust-lang#138548 - jieyouxu:rollup-xpslct5, r=jieyouxu
66678e6 
Rollup of 16 pull requests

Successful merges:

 - rust-lang#133055 (Expand `CloneToUninit` documentation.)
 - rust-lang#137147 (Add exclude to config.toml)
 - rust-lang#137864 (Don't drop `Rvalue::WrapUnsafeBinder` during GVN)
 - rust-lang#137890 (doc: clarify that consume can be called after BufReader::peek)
 - rust-lang#137956 (Add RTN support to rustdoc)
 - rust-lang#137968 (Properly escape regexes in Python scripts)
 - rust-lang#138082 (Remove `#[cfg(not(test))]` gates in `core`)
 - rust-lang#138275 (expose `is_s390x_feature_detected!` from `std::arch`)
 - rust-lang#138303 (Fix Ptr inconsistency in {Rc,Arc})
 - rust-lang#138309 (Add missing doc for intrinsic (Fix PR135334))
 - rust-lang#138323 (Expand and organize `offset_of!` documentation.)
 - rust-lang#138329 (debug-assert that the size_hint is well-formed in `collect`)
 - rust-lang#138465 (linkchecker: bump html5ever)
 - rust-lang#138471 (Clean up some tests in tests/ui)
 - rust-lang#138472 (Add codegen test for rust-lang#129795)
 - rust-lang#138484 (Use lit span when suggesting suffix lit cast)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4946818 into rust-lang:master Mar 16, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 16, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2025
@rust-timer
Unrolled build for rust-lang#138323
f02bb61 
Rollup merge of rust-lang#138323 - kpreid:offset-of-doc, r=Mark-Simulacrum

Expand and organize `offset_of!` documentation.

* Give example of how to get the offset of an unsized tail field (prompted by discussion <rust-lang#133055 (comment)>).
* Specify the return type.
* Add section headings.
* Reduce “Visibility is respected…”, to a single sentence.
* Move `offset_of_enum` documentation to unstable book (with link to it).
* Add `offset_of_slice` documentation in unstable book.

r? Mark-Simulacrum
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2025
@rust-timer
Unrolled build for rust-lang#133055
69a2c97 
Rollup merge of rust-lang#133055 - kpreid:clone-uninit-doc, r=Mark-Simulacrum

Expand `CloneToUninit` documentation.

* Clarify relationship to `dyn` after rust-lang#133003.
* Add an example of using it with `dyn` as rust-lang#133003 enabled.
* Replace parameter name `dst` with `dest` to avoid confusion between abbreviations for “DeSTination” and “Dynamically-Sized Type”.
* Add an example of implementing it.
* Add links to Rust Reference for the mentioned concepts.
* Mention that its method should rarely be called.
* Various small corrections.

Please review the `unsafe` code closely, as I am not an expert in the best possible ways to express these operations. (It might also be better to omit the implementation example entirely.)

cc `@zachs18` rust-lang#126799
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Mar 19, 2025
@kpreid
Expand and organize offset_of! documentation.
47a68bb 
* Give example of how to get the offset of an unsized tail field
  (prompted by discussion <rust-lang#133055 (comment)>).
* Specify the return type.
* Add section headings.
* Reduce “Visibility is respected…”, to a single sentence.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Mar 19, 2025
@jieyouxu
Rollup merge of rust-lang#133055 - kpreid:clone-uninit-doc, r=Mark-Si…
3f4f42a 
…mulacrum

Expand `CloneToUninit` documentation.

* Clarify relationship to `dyn` after rust-lang#133003.
* Add an example of using it with `dyn` as rust-lang#133003 enabled.
* Replace parameter name `dst` with `dest` to avoid confusion between abbreviations for “DeSTination” and “Dynamically-Sized Type”.
* Add an example of implementing it.
* Add links to Rust Reference for the mentioned concepts.
* Mention that its method should rarely be called.
* Various small corrections.

Please review the `unsafe` code closely, as I am not an expert in the best possible ways to express these operations. (It might also be better to omit the implementation example entirely.)

cc `@zachs18` rust-lang#126799
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Mar 19, 2025
@jieyouxu
Rollup merge of rust-lang#138323 - kpreid:offset-of-doc, r=Mark-Simul…
8112a0c 
…acrum

Expand and organize `offset_of!` documentation.

* Give example of how to get the offset of an unsized tail field (prompted by discussion <rust-lang#133055 (comment)>).
* Specify the return type.
* Add section headings.
* Reduce “Visibility is respected…”, to a single sentence.
* Move `offset_of_enum` documentation to unstable book (with link to it).
* Add `offset_of_slice` documentation in unstable book.

r? Mark-Simulacrum
tautschnig pushed a commit to model-checking/verify-rust-std that referenced this pull request Mar 19, 2025
@kpreid @tautschnig
Expand and organize offset_of! documentation.
8ba25cd 
* Give example of how to get the offset of an unsized tail field
  (prompted by discussion <rust-lang#133055 (comment)>).
* Specify the return type.
* Add section headings.
* Reduce “Visibility is respected…”, to a single sentence.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum

@zachs18 zachs18

Assignees

@Mark-Simulacrum Mark-Simulacrum

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@kpreid @rustbot @rust-log-analyzer @bors @Mark-Simulacrum @zachs18 @cuviper