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

Implement MaybeUninit::fill{,_cloned,_mut,_with,_from} #117426

Closed
wants to merge 1 commit into from

Conversation

jmillikin
Copy link
Contributor

@jmillikin jmillikin commented Oct 31, 2023

ACP: rust-lang/libs-team#156

Tracking issue: #117428

@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2023

r? @joshtriplett

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 31, 2023
@jmillikin
Copy link
Contributor Author

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api 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 Oct 31, 2023
@jmillikin
Copy link
Contributor Author

jmillikin commented Oct 31, 2023

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 &T to have better control over Drop handling.
    • To match existing test behavior I try to be careful about how many drop() calls happen. There was no good way to avoid dropping value if 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.
  • 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 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.

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().

@jmillikin
Copy link
Contributor Author

r? @m-ou-se

@rustbot rustbot assigned m-ou-se and unassigned joshtriplett Nov 24, 2023
@Amanieu
Copy link
Member

Amanieu commented Dec 16, 2023

* 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.

@Amanieu Amanieu self-assigned this Dec 16, 2023
@bors
Copy link
Contributor

bors commented Feb 16, 2024

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

@Amanieu
Copy link
Member

Amanieu commented Feb 29, 2024

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 29, 2024
@Amanieu
Copy link
Member

Amanieu commented Mar 20, 2024

ping @jmillikin, could you address the review feedback?

@jmillikin
Copy link
Contributor Author

Closed in favor of #121280

@jmillikin jmillikin closed this Mar 21, 2024
@jmillikin jmillikin deleted the maybe-uninit-fill branch March 21, 2024 00:21
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. 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.

6 participants