Make std::mem::transmute_copy accept ?Sized inputs by nvzqz · Pull Request #112457 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This enables conversions from dynamically-sized types like [u8].
r? @scottmcm
(rustbot has picked a reviewer for you, use r? to override)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
Relevant to the library API team, which will review and decide on the PR/issue.
and removed T-libs
Relevant to the library team, which will review and decide on the PR/issue.
labels
This enables conversions from dynamically-sized types like [u8].
This change is insta-stable, so needs a completed FCP to proceed.
label
Comment on lines 1056 to 1059
| assert!( |
|---|
| size_of::() >= size_of::(), |
| size_of_val(src) >= size_of::(), |
| "cannot transmute_copy if Dst is larger than Src" |
| ); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer guaranteed to be compiled out in --release. I do not believe we want this assert in this case.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to debug_assert! in 3f1825a
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not guaranteed to be compiled out?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tbu- because size_of_val for dynamically-sized types requires a runtime check since the type's size is not known at compile-time
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this to debug_assert! means this check no longer triggers for 99% of users who do not work with a debug build of the stdlib.
Code that uses this function is likely performance-sensitive, so we do not want to emit an assert for dynamically-sized types in release mode.
Transmute only requires the bits to be valid for Dst Src, not necesarily that Dst Src is in a safe state.
Do we have any requirements on custom DSTs that their size must be computable at all times? One might get funny effects if such a type is trying to compute its size in the middle of unsafe code mucking with its bytes.
@the8472 size_of::<Dst>() emits a compile-time constant, so I don't see how one would get funny effects at runtime
Hrrm, right, only the source is unsized. Still, the assert would try to compute the dynamic size of the source in debug builds of the standard library.
Flipping this over to a libs-api reviewer since this needs an FCP due to being new stable capabilty
r? @m-ou-se
Tangentially: ptr::read will be const fn stable in 1.71.
Out of curiosity, do you have any uses in mind for anything but [u8]? For example, using this with trait objects feels much weirder than with byte slices.
But coming from [u8], bytes.as_ptr().cast::<Target>().read_unaligned() would also work fine -- from a byte slice using the align-1 version is always fine -- and that makes me a bit unconvinced that changing transmute_copy is the right choice.
@nvzqz would be nice to have a reply to #112457 (comment) and can resolve the conflicts if interested in moving this forward? thanks
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Labels
This change is insta-stable, so needs a completed FCP to proceed.
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the library API team, which will review and decide on the PR/issue.