-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
use const array repeat expressions for uninit_array #62799
Conversation
src/libcore/mem/maybe_uninit.rs
Outdated
@@ -248,6 +248,7 @@ impl<T> MaybeUninit<T> { | |||
/// [type]: union.MaybeUninit.html | |||
#[stable(feature = "maybe_uninit", since = "1.36.0")] | |||
#[inline(always)] | |||
#[rustc_promotable] |
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.
Does this have support for feature gating?
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.
I don't think so. @oli-obk ?
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.
Explicitly puting the MaybeUninit::uninit()
seems to work. That would make this unnecessary.
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.
That playground does not load here? I just get a blank page. The one at https://play.rust-lang.org/ works.
Can you paste the code you are suggesting here in GitHub?
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 the playground at integer32 broken for me too.
The code was:
use std::mem::MaybeUninit;
fn main() {
const A: MaybeUninit<u8> = MaybeUninit::uninit();
let a_arr = [A; 2];
}
But looking at the optimized llvm ir, it seems that llvm emits a memset
with zero, instead of keeping it uninitialized in case of:
use std::mem::MaybeUninit;
pub fn abc() -> [MaybeUninit<u8>; 16] {
const A: MaybeUninit<u8> = MaybeUninit::uninit();
let a_arr = [A; 16]; //unsafe { std::mem::uninitialized() };
a_arr
}
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.
MaybeUninit<u8>
is Copy
, so this does not really test the new code paths anyway.
But yes, you can probably "trigger" promotion as usual with a manual const
.
This should not be necessary. You can use a perma-unstable associated constant instead: impl<T> MaybeUninit<T> {
#[unstable(
feature = "internal_uninit_const",
issue = "0",
reason = "hack to work around promotability",
)]
pub const UNINIT: Self = Self::uninit();
}
fn foo<T>() {
let x = [MaybeUninit::<T>::UNINIT; 5]; // OK.
} (I checked that this idea works on master as of the time of writing this comment.) |
@Centril The goal is to remove the macro ASAP. On Zulip I suggested we treat zero-argument |
Ah yes an explicit constant should do it, true. That won't help users outside libstd but that is a separate concern.
I don't think I am a fan of expanding the scope of promotion in any way. |
Okay, this no longer makes |
This comment has been minimized.
This comment has been minimized.
But there is a |
No reason we couldn't! Or even for any sub-computation that doesn't depend on arguments. |
To work around the lack of |
I'm happy with this so r=me rollup if you like or wait for Oliver's review otherwise, up to you. ;) |
@bors r=Centril rollup |
📌 Commit f3abbf7 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1500, this pull request will be tested once the tree is reopened |
use const array repeat expressions for uninit_array With a first implementation of rust-lang#49147 having landed, we can make this macro nicer and phase it out with the next bootstrap bump. However, to make this work, we have to mark `MaybeUninit::uninit()` as promotable. I do feel uneasy about promoting stuff involving uninitialized memory, but OTOH no *operation* on `MaybeUninit` is promotable, so maybe this is okay? r? @oli-obk @eddyb
Rollup of 14 pull requests Successful merges: - #62709 (Test that maplike FromIter satisfies uniqueness) - #62713 (Stabilize <*mut _>::cast and <*const _>::cast) - #62746 ( do not use assume_init in std::io) - #62787 (Fix typo in src/libstd/net/udp.rs doc comment) - #62788 (normalize use of backticks in compiler messages for libcore/ptr) - #62799 (use const array repeat expressions for uninit_array) - #62810 (normalize use of backticks in compiler messages for librustc_lint) - #62812 (normalize use of backticks in compiler messages for librustc_metadata) - #62832 (normalize use of backticks in compiler messages for librustc_incremental) - #62845 (read: fix doc comment) - #62853 (normalize use of backticks in compiler messages for librustc/hir) - #62854 (Fix typo in Unicode character name) - #62858 (Change wrong variable name.) - #62870 (fix lexing of comments with many \r) Failed merges: r? @ghost
With a first implementation of #49147 having landed, we can make this macro nicer and phase it out with the next bootstrap bump.
However, to make this work, we have to mark
MaybeUninit::uninit()
as promotable. I do feel uneasy about promoting stuff involving uninitialized memory, but OTOH no operation onMaybeUninit
is promotable, so maybe this is okay?r? @oli-obk @eddyb