alloc: Add new_zeroed() versions like new_uninit(). #66128
Conversation
MaybeUninit has both uninit() and zeroed(), it seems reasonable to have the same surface on Box/Rc/Arc. Needs tests.
|
cc @SimonSapin |
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @SimonSapin |
| pub fn new_zeroed() -> Box<mem::MaybeUninit<T>> { | ||
| unsafe { | ||
| let mut uninit = Self::new_uninit(); | ||
| ptr::write_bytes::<T>(uninit.as_mut_ptr(), 0, 1); |
There was a problem hiding this comment.
How about using alloc_zeroed when creating the Box so you don't need to manually zero the memory? It might make sense to do the same for Rc and Arc as well.
There was a problem hiding this comment.
For Rc / Arc it was a bit harder because the refcount is part of the allocation, so you don't really want to do that. You could, but then you have to duplicate a lot more code, which is not great.
For Box, I guess I can indeed do that without too much effort, though it's still more code and not 100% sure if worth it.
I get the consistency argument, but do you have concrete use cases? |
Centril
changed the title
It was mostly for consistency, but I happen to, kinda! The reason I thought of this is because I was replacing this piece of firefox code full of undefined behavior. What it does is allocating a bunch of C++ structs in an These C++ structs do contain various things which have Arguably that was quite a bit footgunny, and that's fixed now using our equivalent to Our internal Arc didn't have this, but if there were more similar use-cases in our code-base I would've opted for adding an equivalent of this. The other reason to add this is that writing it on your own is slightly footgunny / really easy to get wrong. See the discussion today here: https://mozilla.logbot.info/servo/20191105#c16736990-c16737025. The initial code by Simon created a reference to an uninitialized value, which is UB as I understand it. Plus it has the one-less-star-zeroes-the-pointer issue. |
(That version created a |
facepalm, indeed |
|
Ping from triage: Thanks! |
|
I'm happy to add tests if people are fine with the addition. |
|
Ping from triage: |
|
Pinging again from triage: |
There was a problem hiding this comment.
r=me
@emilio Do you still have changes you want to make? If not please remove [WIP] from the PR title and let’s land this.
SimonSapin
added
S-waiting-on-author
emilio
changed the title
|
Ah, I wanted to add tests, but I forgot that doctests do actually run, so this should be good to go. |
|
Alright! @bors r+ |
|
📌 Commit b12e142 has been approved by |
bors
added
S-waiting-on-bors
alloc: Add new_zeroed() versions like new_uninit(). MaybeUninit has both uninit() and zeroed(), it seems reasonable to have the same surface on Box/Rc/Arc. Needs tests. cc rust-lang#63291
Rollup of 14 pull requests Successful merges: - #66128 (alloc: Add new_zeroed() versions like new_uninit().) - #66661 (Add riscv64gc-unknown-linux-gnu target) - #66663 (Miri: print leak report even without tracing) - #66711 (Add hardware floating point features to aarch64-pc-windows-msvc) - #66713 (introduce a target to build the kernel of the unikernel HermitCore) - #66717 (tidy: Accommodate rustfmt's preferred layout of stability attributes) - #66719 (Store pointer width as u32 on Config) - #66720 (Move ErrorReported to rustc_errors) - #66737 (Error codes cleanup) - #66754 (Various tweaks to diagnostic output) - #66763 (Minor edit for documentation-tests.md that increases clarity) - #66779 (follow the same function order in the trait) - #66786 (Add wildcard test for const_if_match) - #66788 (Allow `Unreachable` terminators through `min_const_fn` checks) Failed merges: r? @ghost
MaybeUninit has both uninit() and zeroed(), it seems reasonable to have the same
surface on Box/Rc/Arc.
Needs tests.
cc #63291