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

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 2 commits into from
+31 −73

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented May 13, 2024 •
edited

[This PR originally contained the changes in #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. The example below could then use let mut buf = [const { MaybeUninit::<u8>::uninit() }; 32];.

The PR adding it also said: #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.

@rustbot
Copy link
Collaborator

rustbot commented May 13, 2024

r? @Nilstrieb

rustbot has assigned @Nilstrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 13, 2024
@ChayimFriedman2
Copy link
Contributor

I still prefer MaybeUninit::uninit_array() to [const { MaybeUninit::uninit() }; N], it's shorter and more readable IMHO.

@workingjubilee
Copy link
Contributor

The examples and the compiler code should probably still prefer the stable option.

Technically the compared-against example was [MaybeUninit::uninit(); N], which implies that the array initializer is a const context.

@Nilstrieb
Copy link
Member

this fell down into the "i hope to get to this eventually but will probably never lol" part of my notification queue and i forgot to look at it again, sorry.
you have my approval for the first two commits, but I'd like a libs-api opinion on the last one. you can split out the first two into a new PR and i'll approve that, or you can keep them all like this if you're okay with letting the first two wait longer (as it will probably take a bit).
r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 4, 2024
@rustbot rustbot assigned dtolnay and unassigned Nilstrieb Jun 4, 2024
@kpreid kpreid mentioned this pull request Jun 4, 2024
Merged
@kpreid
Copy link
Contributor Author

kpreid commented Jun 4, 2024

Split out the first two parts into #125995. I'll re-summarize this PR once that's merged.

fmease added a commit to fmease/rust that referenced this pull request Jun 4, 2024
@fmease
Rollup merge of rust-lang#125995 - kpreid:const-uninit-stable, r=Nils…
b6117c6 
…trieb

Use inline const blocks to create arrays of `MaybeUninit`.

This PR contains 2 changes enabled by the fact that [`inline_const` is now stable](rust-lang#104087), and was split out of rust-lang#125082.

1. Use inline const instead of `unsafe` to construct arrays in `MaybeUninit` examples.

   Rationale: Demonstrate good practice of avoiding `unsafe` code where it is not strictly necessary.

4. Use inline const instead of `unsafe` to implement `MaybeUninit::uninit_array()`.

    This is arguably giving the compiler more work to do, in exchange for eliminating just one single internal unsafe block, so it's less certain that this is good on net.

r​? `@Nilstrieb`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 5, 2024
@workingjubilee
Rollup merge of rust-lang#125995 - kpreid:const-uninit-stable, r=Nils…
6b6b698 
…trieb

Use inline const blocks to create arrays of `MaybeUninit`.

This PR contains 2 changes enabled by the fact that [`inline_const` is now stable](rust-lang#104087), and was split out of rust-lang#125082.

1. Use inline const instead of `unsafe` to construct arrays in `MaybeUninit` examples.

   Rationale: Demonstrate good practice of avoiding `unsafe` code where it is not strictly necessary.

4. Use inline const instead of `unsafe` to implement `MaybeUninit::uninit_array()`.

    This is arguably giving the compiler more work to do, in exchange for eliminating just one single internal unsafe block, so it's less certain that this is good on net.

r​? `@Nilstrieb`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
@rust-timer
Unrolled build for rust-lang#125995
e55c438 
Rollup merge of rust-lang#125995 - kpreid:const-uninit-stable, r=Nilstrieb

Use inline const blocks to create arrays of `MaybeUninit`.

This PR contains 2 changes enabled by the fact that [`inline_const` is now stable](rust-lang#104087), and was split out of rust-lang#125082.

1. Use inline const instead of `unsafe` to construct arrays in `MaybeUninit` examples.

   Rationale: Demonstrate good practice of avoiding `unsafe` code where it is not strictly necessary.

4. Use inline const instead of `unsafe` to implement `MaybeUninit::uninit_array()`.

    This is arguably giving the compiler more work to do, in exchange for eliminating just one single internal unsafe block, so it's less certain that this is good on net.

r​? `@Nilstrieb`
@bors
Copy link
Contributor

bors commented Jun 5, 2024

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

@kpreid
Replace MaybeUninit::uninit_array() with array repeat expression.
d1c71d6 
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 kpreid changed the title Use inline const blocks to create arrays of MaybeUninit; remove uninit_array(). Remove MaybeUninit::uninit_array() and replace it with inline const blocks. Jun 5, 2024
@kpreid
Copy link
Contributor Author

kpreid commented Jun 5, 2024

This PR now consists solely of removing MaybeUninit::uninit_array().

@dtolnay dtolnay mentioned this pull request Jun 5, 2024
Open
6 tasks
dtolnay
dtolnay reviewed Jun 5, 2024
View reviewed changes
compiler/rustc_data_structures/src/sip128.rs Outdated Show resolved Hide resolved
library/core/src/fmt/float.rs Outdated Show resolved Hide resolved
library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
@dtolnay dtolnay removed O-windows Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 5, 2024
@rustbot rustbot added the O-windows Operating system: Windows label Jun 5, 2024
dtolnay
dtolnay approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

I am attempting to start a T-libs-api FCP for this in #96097 (comment).

https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/rfcbot.20asleep

@dtolnay dtolnay added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@kpreid @rustbot @ChayimFriedman2 @workingjubilee @Nilstrieb @bors @dtolnay