-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Add MaybeUninit array transpose impls #102023
Conversation
r? @scottmcm (rust-highfive has picked a reviewer for you, use r? to override) |
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 |
I'm definitely in favor of adding this, but I think it needs an ACP. |
@thomcc Sounds good, done: rust-lang/libs-team#110 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hmmm, no idea how those failures are related. Will ignore for now. |
Yeah that's a strange error, can you rebase and force push? |
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.
The transmutation itself is sound. This is insta-stable though so definitely needs careful T-libs review.
b08cc51
to
1699082
Compare
4c3f901
to
11037a3
Compare
Looks good, apart from some nits. But I can't judge if |
11037a3
to
62dae1f
Compare
Sweet, thanks for the review! :) |
@rustbot label +needs-fcp |
This comment was marked as outdated.
This comment was marked as outdated.
cd7c8fe
to
ee9ac14
Compare
r? @scottmcm I think this can go back to you now that these are going through the standard nightly process. |
library/core/src/mem/maybe_uninit.rs
Outdated
// SAFETY: T and MaybeUninit<T> have the same layout | ||
let result = unsafe { super::transmute_copy(&self) }; | ||
super::forget(self); | ||
result |
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.
suggestion: while this works fine, since none of it can panic, we try in the docs to follow the "pre-pit your peaches" order of things, rather than "forget it afterwards", and consider forget
soft-deprecated.
So please update this to something like
pub fn transpose(self) -> [MaybeUninit<T>; N] {
// SAFETY: T, ManuallyDrop<T>, and MaybeUninit<T> have the same layout
unsafe { super::transmute_copy(&ManuallyDrop::new(self)) }
}
(And ditto for the one below)
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.
Oh yeah, that's so much better. Is there a reason forget
isn't deprecated? It seems like you could always replace it with let _ = ManuallyDrop::new(foo);
(or drop(ManuallyDrop::new(foo));
) worse case scenario.
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.
Yeah, you can -- in fact that's literally what forget
does
rust/library/core/src/mem/mod.rs
Lines 153 to 155 in b8c35ca
pub const fn forget<T>(t: T) { | |
let _ = ManuallyDrop::new(t); | |
} |
I think it's not deprecated because, unlike mem::uninitialized
, there's lots of code that used forget
that's still perfectly correct because it only does things (like copy_nonoverlapping
) that can't panic.
But maybe it's time to start thinking about it, now that ManuallyDrop
has been around for long enough. Or at least a deprecated-in-rust-2.0 for now so it gets picked up by
rust/compiler/rustc_lint_defs/src/builtin.rs
Lines 2243 to 2258 in 9b0a099
declare_lint! { | |
/// The `deprecated_in_future` lint is internal to rustc and should not be | |
/// used by user code. | |
/// | |
/// This lint is only enabled in the standard library. It works with the | |
/// use of `#[deprecated]` with a `since` field of a version in the future. | |
/// This allows something to be marked as deprecated in a future version, | |
/// and then this lint will ensure that the item is no longer used in the | |
/// standard library. See the [stability documentation] for more details. | |
/// | |
/// [stability documentation]: https://rustc-dev-guide.rust-lang.org/stability.html#deprecated | |
pub DEPRECATED_IN_FUTURE, | |
Allow, | |
"detects use of items that will be deprecated in a future version", | |
report_in_external_macro | |
} |
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.
Hmm, maybe not. It looks like mem::forget(guard);
is still very common in the library for panic guards, where there's no need to consume them like in the example, and thus the forget
phrasing is kinda nice.
But maybe that also just means that the PanicGuard
types should have a defuse
method or something as part of its API instead of telling people to forget
it.
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.
Hmmm, yeah I see the appeal for panic guards, but poking around the stdlib I think it's actually pretty hard to understand what's going on unless you know what the guards are for, so maybe encouraging people to write a defuse
method or add a comment explaining the weirdness is a good thing. Created #103101.
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
ee9ac14
to
393434c
Compare
Thanks! These look good for nightly to me. @bors r+ |
Rollup of 6 pull requests Successful merges: - rust-lang#101717 (Add documentation about the memory layout of `UnsafeCell<T>`) - rust-lang#102023 (Add MaybeUninit array transpose From impls) - rust-lang#103033 (Update pkg-config) - rust-lang#103080 (pretty: fix to print some lifetimes on HIR pretty-print) - rust-lang#103082 (Surround type with backticks) - rust-lang#103088 (Fix settings page) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The PR title is outdated now, right? |
Yup, fixed. |
Is |
@SimonSapin Yes, because EDIT: Fixed! Thanks @thomcc! |
(I think you mean transmute, and only mention it because it might be pretty confusing with the mistake) |
See discussion in #101179 and #96097. I believe this solution offers the simplest implementation with minimal future API regret. Closes rust-lang/libs-team#110.
@RalfJung mind doing a correctness review?