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

MaybeUninit inherent slice methods part 2 #135394

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Jan 12, 2025

These were moved out of #129259 since they require additional libs-api approval. Tracking issue: #117428.

New API surface:

impl<T> [MaybeUninit<T>] {
    // replacing fill; renamed to avoid conflict
    pub fn write_filled(&mut self, value: T) -> &mut [T] where T: Clone;

    // replacing fill_with; renamed to avoid conflict
    pub fn write_with<F>(&mut self, value: F) -> &mut [T] where F: FnMut() -> T;

    // renamed to remove "fill" terminology, since this is closer to the write_*_of_slice methods
    pub fn write_iter<I>(&mut self, iter: I) -> (&mut [T], &mut Self) where I: Iterator<Item = T>;
}

Relevant motivation for these methods; see #129259 for earlier methods' motiviations.

  • I chose write_filled since filled is being used as an object here, whereas it's being used as an action in fill.
  • I chose write_with instead of write_filled_with since it's shorter and still matches well.
  • I chose write_iter because it feels completely different from the fill methods, and still has the intent clear.

In all of the methods, it felt appropriate to ensure that they contained write to clarify that they are effectively just special ways of doing MaybeUninit::write for each element of a slice.

Tracking issue: #117428

r? libs-api

@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 Jan 12, 2025
@clarfonthey
Copy link
Contributor Author

@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2025
@clarfonthey clarfonthey marked this pull request as ready for review January 12, 2025 05:03
@clarfonthey
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jan 12, 2025
@bors
Copy link
Contributor

bors commented Jan 27, 2025

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

@tgross35
Copy link
Contributor

@rustbot label +I-libs-api-nominated

This PR renamed fill, fill_with and fill_from to write_filled, write_with, and write_iter and makes them inherent. Making these inherent was already approved but not the exact names. Some change has to happen because fill and fill_with conflict with [T]::fill{,_with}.

(Personally I prefer keeping fill and fill_with in the names to be consistent with slice, along the lines of Amanieu's fill_init suggestion at #129259 (comment).)

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 21, 2025
@RalfJung
Copy link
Member

RalfJung commented Feb 22, 2025

If we're picking new names anyway, can we also change the type of the closure and give them a usize argument so the initial value can depend on the index, a la https://doc.rust-lang.org/std/array/fn.from_fn.html ?

@RalfJung
Copy link
Member

RalfJung commented Feb 22, 2025

@clarfonthey please always include a reference to the tracking issue in the PR description for changes to an unstable API; otherwise it can become quite tricky to track the changes to a feature.

@clarfonthey
Copy link
Contributor Author

Ah, you're totally right; these are all part of a single tracking issue that is now linked in the description.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Feb 23, 2025

(Personally I prefer keeping fill and fill_with in the names to be consistent with slice, along the lines of Amanieu's fill_init suggestion at #129259 (comment).)

Also adding context that may be missed because the PR was split: copy_from_slice and clone_from_slice were renamed to write_copy_of_slice and write_clone_of_slice to add "write" in the name, so, unless renaming those is also desired, we would probably want to go with write_filled and write_filled_with instead of fill_init and fill_with_init.

In terms of fill_from, I especially prefer the name write_iter because, among other things, it actually might not full the entire slice if the iterator is too short. The result returned will be filled, but, it's not necessarily the length of the buffer, which could be confusing.

Copy link
Contributor

@Sky9x Sky9x left a comment

Choose a reason for hiding this comment

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

Please make sure these deprecated unstable methods do actually eventually get removed at some point (couple release cycles). The last time unstable methods were deprecated (pointer to_bits and from_bits), they were forgotten for 2 years (see #127071).

@Amanieu
Copy link
Member

Amanieu commented Feb 25, 2025

We discussed this in the libs-api meeting. We're mostly happy with the names for now, but this could be re-visited before stabilzation. Also we agree with @RalfJung's suggestion to add an index to the closure.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 25, 2025
@clarfonthey clarfonthey force-pushed the uninit-slices-part-2 branch from 6a34d58 to fa0164e Compare March 8, 2025 13:25
@clarfonthey
Copy link
Contributor Author

Finally got around to this and made the requested change to add an index to the closure for write_with. Should be ready to review now.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the uninit-slices-part-2 branch from fa0164e to 16acb5a Compare March 8, 2025 13:51
@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the uninit-slices-part-2 branch from 16acb5a to 372e1d4 Compare March 8, 2025 14:02
@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the uninit-slices-part-2 branch from 372e1d4 to 996da14 Compare March 8, 2025 14:43
@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the uninit-slices-part-2 branch from 996da14 to 12a8599 Compare March 8, 2025 14:54
@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the uninit-slices-part-2 branch from 12a8599 to ccb137b Compare March 8, 2025 15:55
@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the uninit-slices-part-2 branch from ccb137b to b707cd9 Compare March 8, 2025 20:57
@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the uninit-slices-part-2 branch from b707cd9 to 8269132 Compare March 8, 2025 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants