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

Split MaybeUninit::write into new feature gate and stabilize it #86344

Merged
merged 4 commits into from
Jul 13, 2021

Conversation

est31
Copy link
Member

@est31 est31 commented Jun 16, 2021

This splits off the MaybeUninit::write function from the maybe_uninit_extra feature gate into a new maybe_uninit_write feature gate and stabilizes it.

Earlier work to improve the documentation of the write function: #86220

Tracking issue: #63567

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(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 Jun 16, 2021
@est31
Copy link
Member Author

est31 commented Jun 16, 2021

Original proposal to stabilize the feature: #63567 (comment)

After that there have been suggestions to split off the write function to stabilize it in isolation.

The const-ness of the function is not stabilized for now because assume_init_mut is not stably const, which is due to assert_inhabited not being stably const. See also #86273.

cc @RalfJung @mgeier @SimonSapin

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Jun 16, 2021
@scottmcm
Copy link
Member

This seems reasonable to me, but it'll need an FCP, so sending to a random libs member:

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned scottmcm Jun 16, 2021
@RalfJung
Copy link
Member

RalfJung commented Jun 16, 2021

No objections from my side, if T-libs agrees that this is useful.

impl<T> MaybeUninit<T> {
  pub fn write(&mut self, val: T) -> &mut T
}

What makes this somewhat non-standard is the fact that write returns something.

@SkiFire13
Copy link
Contributor

What makes this somewhat non-standard is the fact that write returns something.

Maybe it could be called init?

@est31
Copy link
Member Author

est31 commented Jun 16, 2021

@RalfJung

What makes this somewhat non-standard is the fact that write returns something.

Indeed there is no analog function called write that also returns something. However, returning the reference is nothing completely novel. E.g. VacantEntry::insert returns a mutable reference after insertion. The newly stable Option::insert and unstable Entry::insert functions do the same (cc #65225), as do the stable Entry::or_insert and Entry::or_insert_with functions. insert might be an alternative name for the write function. I prefer write though as it shares a small (safe) footgun with ptr::write: if you call the function twice, no drop is executed for the first content. Insert doesn't have this footgun. write is also safe sugar for the unsafe .as_mut_ptr().write(). Those two reasons speak in favour of calling it write I think.

As for init, I think the best argument that speaks in favour is the linguistic relationship to the MaybeUninit name. By calling the init function, the maybe-un-init is now initialized. I think there is confusion potential with the new function though, and I can't find any other function named init in the standard library.

@est31
Copy link
Member Author

est31 commented Jun 22, 2021

@rustbot label S-waiting-on-team

@rustbot rustbot added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jun 22, 2021
@ghost
Copy link

ghost commented Jun 24, 2021

Should "the destructor is not ran for the inner data" say run instead of ran at the top of the docs for write?

https://github.com/rust-lang/rust/blob/4aaaa424727a9bb02b494d34e1cc408e1de876f2/library/core/src/mem/maybe_uninit.rs#L412-L413

@est31
Copy link
Member Author

est31 commented Jun 24, 2021

@skippy10110 it seems you are right. "the destructor is not ran for the inner data" seems to be a present perfect passive construction, which means it needs to use the past participle form of run which is run.

https://www.wallstreetenglish.com/blog/the-present-perfect-tense-and-the-passive-voice/

https://dictionary.cambridge.org/dictionary/english/run

Good catch!

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

I love the example! Thank you for digging that up. I suggested a few edits to make it a smidge clearer IMO. :-)

library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved
est31 added 2 commits June 25, 2021 00:54
present perfect passive constructions need to use the past participle form,
which for run is "run".
@est31 est31 force-pushed the maybe-uninit-extra branch from 9640cb3 to 8e328be Compare June 24, 2021 22:54
@m-ou-se m-ou-se added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 7, 2021
@RalfJung
Copy link
Member

RalfJung commented Jul 12, 2021

FCP passed, so I think this is ready to land. :)

However, I think there are some more examples that could use the new method. In the docs for MaybeUninit itself, we have x.as_mut_ptr().write and *elem = MaybeUninit::new; I think both of these would be written more ergonomically with write, wouldn't they?

@est31
Copy link
Member Author

est31 commented Jul 12, 2021

r? @dtolnay

@RalfJung
Copy link
Member

Based on the example review by @BurntSushi
@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2021

📌 Commit 848a621 has been approved by RalfJung

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 12, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 12, 2021
@JohnTitor JohnTitor added relnotes Marks issues that should be documented in the release notes of the next release. and removed S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Jul 12, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 13, 2021
Rollup of 11 pull requests

Successful merges:

 - rust-lang#86344 (Split MaybeUninit::write into new feature gate and stabilize it)
 - rust-lang#86811 (Remove unstable `io::Cursor::remaining`)
 - rust-lang#86846 (stdio_locked: add tracking issue)
 - rust-lang#86887 (rustdoc: remove dead code in `clean`)
 - rust-lang#87007 (Fix rust-analyzer install when not available.)
 - rust-lang#87035 (Fix implementors display)
 - rust-lang#87065 (Fix ICE with unsized type in const pattern)
 - rust-lang#87070 (Simplify future incompatible reporting.)
 - rust-lang#87077 (:arrow_up: rust-analyzer)
 - rust-lang#87078 (Rustdoc: suggest removing disambiguator if linking to field)
 - rust-lang#87089 (CTFE engine: small cleanups)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Jul 13, 2021

⌛ Testing commit 848a621 with merge 14c0c3e...

@bors bors merged commit b507cd1 into rust-lang:master Jul 13, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 13, 2021
@jplatte jplatte mentioned this pull request Jul 22, 2021
65 tasks
LeSeulArtichaut added a commit to LeSeulArtichaut/rust that referenced this pull request Jul 24, 2021
… r=LeSeulArtichaut

DOC: remove unnecessary feature crate attribute from example code

I'm not sure whether I fully understand the stabilization process (I most likely don't), but I think this attribute isn't necessary here, right?

This was recently stabilized in rust-lang#86344.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 24, 2021
… r=LeSeulArtichaut

DOC: remove unnecessary feature crate attribute from example code

I'm not sure whether I fully understand the stabilization process (I most likely don't), but I think this attribute isn't necessary here, right?

This was recently stabilized in rust-lang#86344.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 24, 2021
… r=LeSeulArtichaut

DOC: remove unnecessary feature crate attribute from example code

I'm not sure whether I fully understand the stabilization process (I most likely don't), but I think this attribute isn't necessary here, right?

This was recently stabilized in rust-lang#86344.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.