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.

Sign up for GitHub

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

update comment at MaybeUninit::uninit_array #83568

Merged
merged 1 commit into from
Mar 30, 2021
Merged

update comment at MaybeUninit::uninit_array #83568

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions library/core/src/mem/maybe_uninit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,9 @@ impl<T> MaybeUninit<T> {
/// Create a new array of `MaybeUninit<T>` items, in an uninitialized state.
///
/// Note: in a future Rust version this method may become unnecessary
/// when array literal syntax allows
/// [repeating const expressions](https://github.com/rust-lang/rust/issues/49147).
/// The example below could then use `let mut buf = [MaybeUninit::<u8>::uninit(); 32];`.
/// when Rust allows
/// [inline const expressions](https://github.com/rust-lang/rust/issues/76001).
/// The example below could then use `let mut buf = [const { MaybeUninit::<u8>::uninit() }; 32];`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it already possible to write let mut buf = [{ const U: MaybeUninit<u8> = MaybeUninit::uninit(); U}; 32]; thus making the method already unnecessary? Or is that too cumbersome to be considered usable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is possible if you have the concrete type; it won't when generic types are involved.

Not sure what people prefer here, for now fixing the docs seemed easier than arguing for the method to be removed. ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, I missed the generic case.

Fixing the docs now is a good idea, I just encountered it earlier this week! But it was for a concrete type. ;)

Copy link
Member

Choose a reason for hiding this comment

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

FYI @tspiteri that appears not to actually uninit.

#![feature(maybe_uninit_uninit_array)]

use std::mem::MaybeUninit;

pub fn f() -> [MaybeUninit<u8>; 32] {
    [{ const U: MaybeUninit<u8> = MaybeUninit::uninit(); U}; 32]
}

pub fn g() -> [MaybeUninit<u8>; 32] {
    MaybeUninit::uninit_array()
}
example::f:
        movq    %rdi, %rax
        xorps   %xmm0, %xmm0
        movups  %xmm0, 16(%rdi)
        movups  %xmm0, (%rdi)
        retq

example::g:
        movq    %rdi, %rax
        retq

Godbolt: https://rust.godbolt.org/z/8nnvPK1PM
A lot like #83657

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @tspiteri that appears not to actually uninit.

Ah, good point, currently uninitialized consts are not actually uninitialized it seems...

Copy link
Member

@bluss bluss Mar 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@RalfJung What about @rodrimati1992's generic code here - for [MaybeUninit<T>; N], which compiles on Rust 1.51 / part of arrayvec 0.7? https://github.com/bluss/arrayvec/blob/7b290b7aa52b0c66b9056cfda577363d005f39b8/src/utils.rs

It seems to contradict what was said about generic types here (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@bluss associated const are the only generic consts we currently have. So yes that is a way to work around this limitation.

This will also run into #83657 though.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's where it was noticed 🙂

///
/// # Examples
///
Expand Down