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.

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 methods uninit_array, slice_get_ref, slice_get_mut #65580

Merged
merged 3 commits into from
Nov 8, 2019

Conversation

SimonSapin
Copy link
Contributor

Eventually these will hopefully become the idiomatic way to work with partially-initialized stack buffers.

All methods are unstable. Note that uninit_array takes a type-level const usize parameter, so it is blocked (at least in its current form) on const generics.

Example:

use std::mem::MaybeUninit;

let input = b"Foo";
let f = u8::to_ascii_uppercase;

let mut buffer: [MaybeUninit<u8>; 32] = MaybeUninit::uninit_array();
let vec;
let output = if let Some(buffer) = buffer.get_mut(..input.len()) {
    buffer.iter_mut().zip(input).for_each(|(a, b)| { a.write(f(b)); });
    unsafe { MaybeUninit::slice_get_ref(buffer) }
} else {
    vec = input.iter().map(f).collect::<Vec<u8>>();
    &vec
};

assert_eq!(output, b"FOO");

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2019
@SimonSapin
Copy link
Contributor Author

CC @rust-lang/libs, @RalfJung

src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
#[inline(always)]
pub fn uninit_array<const LEN: usize>() -> [Self; LEN] {
unsafe {
MaybeUninit::<[MaybeUninit<T>; LEN]>::uninit().assume_init()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a bummer. We should be able to use:

[Self::UNINIT; LEN]

here were it not for the fact that we would get "error: array lengths can't depend on generic parameters" (solvable by e.g. allowing _ as the array length -- https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=28015a7e1c5856eef0ee32f4d53206a5).

Can you leave a FIXME for now?

@RalfJung
Copy link
Member

The API itself seems reasonable (though of get_ref and friends are renamed, likely the slice methods should, too).

However, I expected to also see something that turns [MaybeUninit<T>; N] into [T; N] -- something like array_assume_init. Why is that not needed?

@SimonSapin
Copy link
Contributor Author

array_assume_init could certainly be added, but some users may want to avoid it because moving large arrays can get expensive. In theory it’s could compile to a no-op, but today’s optimizer often has trouble eliding redundant copies. (Compare with Box::<[MaybeUninit<_>]>::assume_init which is a pointer cast even without optimizations.) So a possible pattern for large-ish buffers is to create them uninitalized on the stack, then only interact with them through borrows.

@Amanieu
Copy link
Member

Amanieu commented Oct 19, 2019

I see the main use case for these to be I/O functions:

// Returns a (possibly smaller) slice of data that was actually read
fn read(buf: &mut [MaybeUninit<u8>]) -> &[u8] {
    unsafe {
        // I'm ignoring any error checking here
        let len = libc::read(fd, buf.as_mut_ptr() as *mut u8, buf.len());
        MaybeUninit::slice_get_ref(&buf[..len])
    }
}

let mut buf: [MaybeUninit<u8>; 32] = MaybeUninit::uninit_array();
let data = read(&mut buf);

@ollie27
Copy link
Member

ollie27 commented Oct 19, 2019

What's the reason for uninit_array when you can already use [MaybeUninit::<T>::uninit(); N] for T: Copy and with #49147 it should work for all types?

@SimonSapin
Copy link
Contributor Author

For T: !Copy, given that #49147 is not available on the stable channel yet, let buffer = unsafe { MaybeUninit::<[MaybeUninit<T>; $N]>::uninit().assume_init() } is a somewhat common pattern today. Having in the standard library avoid the unsafe block in user code.

I didn’t realize that #49147 would apply here, but you’re right that if it’s stabilized soon enough maybe it’s not worth having a standard library method that will be replaceable with let buffer = [MaybeUninit::<T>::uninit(); $N];

@JohnCSimon
Copy link
Member

Ping from triage
@withoutboats can you please review this PR
cc: @SimonSapin
Thank you!

@Amanieu
Copy link
Member

Amanieu commented Oct 26, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2019

📌 Commit 2cb52a628868f0307f5a5da0056760e695cc64c2 has been approved by Amanieu

@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 Oct 26, 2019
src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
src/libcore/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Oct 26, 2019

@bors p=-1 rollup=never until the docs are fixed

@Centril
Copy link
Contributor

Centril commented Oct 30, 2019

@bors r- p=0 rollup=maybe

@SimonSapin Can you please fix the docs so that they are consistent? :) Thanks.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 30, 2019
@hdhoang
Copy link
Contributor

hdhoang commented Nov 7, 2019

ping from triage @SimonSapin, can you address the reviewer's comment? thanks!

@SimonSapin
Copy link
Contributor Author

This week is rather packed but I’ll get around to it. Sorry for the delays!

@Centril
Copy link
Contributor

Centril commented Nov 7, 2019

@SimonSapin Do you mind if I help out and fix the issue locally and push to your branch?

@SimonSapin
Copy link
Contributor Author

Not at all, go ahead!

@Centril
Copy link
Contributor

Centril commented Nov 7, 2019

@bors r=Amanieu rollup

@bors
Copy link
Contributor

bors commented Nov 7, 2019

📌 Commit 639c4f7 has been approved by Amanieu

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 7, 2019
…nieu

Add `MaybeUninit` methods `uninit_array`, `slice_get_ref`, `slice_get_mut`

Eventually these will hopefully become the idiomatic way to work with partially-initialized stack buffers.

All methods are unstable. Note that `uninit_array` takes a type-level `const usize` parameter, so it is blocked (at least in its current form) on const generics.

Example:

```rust
use std::mem::MaybeUninit;

let input = b"Foo";
let f = u8::to_ascii_uppercase;

let mut buffer: [MaybeUninit<u8>; 32] = MaybeUninit::uninit_array();
let vec;
let output = if let Some(buffer) = buffer.get_mut(..input.len()) {
    buffer.iter_mut().zip(input).for_each(|(a, b)| { a.write(f(b)); });
    unsafe { MaybeUninit::slice_get_ref(buffer) }
} else {
    vec = input.iter().map(f).collect::<Vec<u8>>();
    &vec
};

assert_eq!(output, b"FOO");
```
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 8, 2019
…nieu

Add `MaybeUninit` methods `uninit_array`, `slice_get_ref`, `slice_get_mut`

Eventually these will hopefully become the idiomatic way to work with partially-initialized stack buffers.

All methods are unstable. Note that `uninit_array` takes a type-level `const usize` parameter, so it is blocked (at least in its current form) on const generics.

Example:

```rust
use std::mem::MaybeUninit;

let input = b"Foo";
let f = u8::to_ascii_uppercase;

let mut buffer: [MaybeUninit<u8>; 32] = MaybeUninit::uninit_array();
let vec;
let output = if let Some(buffer) = buffer.get_mut(..input.len()) {
    buffer.iter_mut().zip(input).for_each(|(a, b)| { a.write(f(b)); });
    unsafe { MaybeUninit::slice_get_ref(buffer) }
} else {
    vec = input.iter().map(f).collect::<Vec<u8>>();
    &vec
};

assert_eq!(output, b"FOO");
```
bors added a commit that referenced this pull request Nov 8, 2019
Rollup of 8 pull requests

Successful merges:

 - #65554 (Enhance the documentation of BufReader for potential data loss)
 - #65580 (Add `MaybeUninit` methods `uninit_array`, `slice_get_ref`, `slice_get_mut`)
 - #66049 (consistent handling of missing sysroot spans)
 - #66056 (rustc_metadata: Some reorganization of the module structure)
 - #66123 (No more hidden elements)
 - #66157 (Improve math log documentation examples)
 - #66165 (Ignore these tests ,since the called commands doesn't exist in VxWorks)
 - #66190 (rustc_target: inline abi::FloatTy into abi::Primitive.)

Failed merges:

 - #66188 (`MethodSig` -> `FnSig` & Use it in `ItemKind::Fn`)

r? @ghost
@bors bors merged commit 639c4f7 into rust-lang:master Nov 8, 2019
@bors
Copy link
Contributor

bors commented Nov 8, 2019

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 8, 2019
@SimonSapin SimonSapin deleted the maybeuninit-array branch November 28, 2019 12:04
kpreid added a commit to kpreid/rust that referenced this pull request Jun 5, 2024
This is possible now that inline const blocks are stable; the idea was
even mentioned as an alternative when `uninit_array()` was added:
<rust-lang#65580 (comment)>

> if it’s stabilized soon enough maybe it’s not worth having a
> standard library method that will be replaceable with
> `let buffer = [MaybeUninit::<T>::uninit(); $N];`

Const array repetition and inline const blocks are now stable (in the
next release), so that circumstance has come to pass, and we no longer
have reason to want `uninit_array()` (unless the repeat syntax is too
annoying).
kpreid added a commit to kpreid/rust that referenced this pull request Jun 24, 2024
This is possible now that inline const blocks are stable; the idea was
even mentioned as an alternative when `uninit_array()` was added:
<rust-lang#65580 (comment)>

> if it’s stabilized soon enough maybe it’s not worth having a
> standard library method that will be replaceable with
> `let buffer = [MaybeUninit::<T>::uninit(); $N];`

Const array repetition and inline const blocks are now stable (in the
next release), so that circumstance has come to pass, and we no longer
have reason to want `uninit_array()` other than convenience. Therefore,
let’s evaluate the inconvenience by not using `uninit_array()` in
the standard library, before potentially deleting it entirely.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 24, 2024
Remove `MaybeUninit::uninit_array()` and replace it with inline const blocks.

\[This PR originally contained the changes in rust-lang#125995 too. See edit history for the original PR description.]

The documentation of `MaybeUninit::uninit_array()` says:

> Note: in a future Rust version this method may become unnecessary when Rust allows [inline const expressions](rust-lang#76001). The example below could then use `let mut buf = [const { MaybeUninit::<u8>::uninit() }; 32];`.

The PR adding it also said: <rust-lang#65580 (comment)>

> if it’s stabilized soon enough maybe it’s not worth having a standard library method that will be replaceable with `let buffer = [MaybeUninit::<T>::uninit(); $N];`

That time has come to pass — inline const expressions are stable — so `MaybeUninit::uninit_array()` is now unnecessary. The only remaining question is whether it is an important enough *convenience* to keep it around.

I believe it is net good to remove this function, on the principle that it is better to compose two orthogonal features (`MaybeUninit` and array construction) than to have a specific function for the specific combination, now that that is possible.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 24, 2024
Rollup merge of rust-lang#125082 - kpreid:const-uninit, r=dtolnay

Remove `MaybeUninit::uninit_array()` and replace it with inline const blocks.

\[This PR originally contained the changes in rust-lang#125995 too. See edit history for the original PR description.]

The documentation of `MaybeUninit::uninit_array()` says:

> Note: in a future Rust version this method may become unnecessary when Rust allows [inline const expressions](rust-lang#76001). The example below could then use `let mut buf = [const { MaybeUninit::<u8>::uninit() }; 32];`.

The PR adding it also said: <rust-lang#65580 (comment)>

> if it’s stabilized soon enough maybe it’s not worth having a standard library method that will be replaceable with `let buffer = [MaybeUninit::<T>::uninit(); $N];`

That time has come to pass — inline const expressions are stable — so `MaybeUninit::uninit_array()` is now unnecessary. The only remaining question is whether it is an important enough *convenience* to keep it around.

I believe it is net good to remove this function, on the principle that it is better to compose two orthogonal features (`MaybeUninit` and array construction) than to have a specific function for the specific combination, now that that is possible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.