Implement MaybeUninit::fill{,_cloned,_mut,_with,_from} by jmillikin · Pull Request #117426 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation9 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
(rustbot has picked a reviewer for you, use r? to override)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
Relevant to the library API team, which will review and decide on the PR/issue.
and removed T-libs
Relevant to the library team, which will review and decide on the PR/issue.
labels
The original ACP proposed adding fill<T: Clone>(&mut self, value: T) -> &mut [T] -- I made the following adjustments to this API:
- Changed the value parameter to
&Tto have better control overDrophandling.- To match existing test behavior I try to be careful about how many
drop()calls happen. There was no good way to avoid droppingvalueif called with an empty slice, but in all other slice sizes the value does not need to be dropped, which made the behavior ambiguous if the parameter is moved vs borrowed.
- To match existing test behavior I try to be careful about how many
- Changed to return a non-mutable slice to avoid the accidental leaking discussed in MaybeUninit::write_slice_cloned() makes it very easy to accidentally leak #80376
- To accommodate the common case of filling with zero or a known bit pattern, I renamed the
Clonefill method tofill_cloned()and addedfill()/fill_mut()forT: Copy-- the mut slice return isn't dangerous since there's no dropping.
The ACP's proposed fill_with<F: FnMut() -> T>(f: F) and fill_from<I: Iterator<Item = T>>(iter: I) were implemented as originally described.
I verified that MaybeUninit<u8>::fill() compiles to memset().
* Changed to return a non-mutable slice to avoid the accidental leaking discussed in [MaybeUninit::write_slice_cloned() makes it very easy to accidentally leak #80376](https://github.com/rust-lang/rust/issues/80376)
I disagree. I think this should always return &mut. I think introducing more API surface will only increase user confusion, and by working with MaybeUninit you already have to be aware of potentially leaking data. All the fill methods should return mutable slices.
* To accommodate the common case of filling with zero or a known bit pattern, I renamed the `Clone` fill method to `fill_cloned()` and added `fill()` / `fill_mut()` for `T: Copy` -- the mut slice return isn't dangerous since there's no dropping.
I'm less concerned about this: there is already fill_with for people who want precise control over how many clone calls are made. The discussion about T vs &T has already happened in #70758, and I would prefer just keeping the T version for consistency with slice::fill.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
ping @jmillikin, could you address the review feedback?
Labels
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the library API team, which will review and decide on the PR/issue.