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

Add resize(_with) and (try_)repeat(_with) for arrays #91506

Closed
wants to merge 2 commits into from

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Dec 4, 2021

mod array {
    pub fn repeat<T: Clone, const N: usize>(value: T) -> [T; N];
    pub fn repeat_with<T, const N: usize>(f: impl FnMut() -> T) -> [T; N];
    pub fn try_repeat_with<R: Try, const N: usize>(
        f: impl FnMut() -> R,
    ) -> ChangeOutputType<R, [R::Output; N]>
    where
        R::Residual: Residual<[R::Output; N]>;
}

impl [T; N] {
    pub fn resize<const NEW_LEN: usize>(self, value: T) -> [T; NEW_LEN] where T: Clone;
    pub fn resize_with<const NEW_LEN: usize>(self, mut f: impl FnMut() -> T) -> [T; NEW_LEN];
}

I tried to do resize_with as just

collect_into_array(IntoIter::new(self).chain(iter::repeat_with(f)))

but unfortunately that optimized very poorly.

As a result there's a bunch of refactoring in here to move the Guard struct that was previously private to try_collect_into_array over to its own module. It's still very much internal -- it's visible only to the core::array module, with no intention of making it the anointed public thing. (And thus it basically only has _unchecked methods.)

This comes with codegen tests, so preemptively
@bors rollup=iffy

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Dec 4, 2021
@scottmcm scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 4, 2021
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the array-resize-repeat branch 2 times, most recently from e66d013 to 019e0e8 Compare December 6, 2021 19:35
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

```rust
mod array {
    pub fn repeat<T: Clone, const N: usize>(value: T) -> [T; N];
}

impl [T; N] {
    pub fn resize<const NEW_LEN: usize>(self, value: T) -> [T; NEW_LEN] where T: Clone;
    pub fn resize_with<const NEW_LEN: usize>(self, mut f: impl FnMut() -> T) -> [T; NEW_LEN];
}
```

I tried to do `resize_with` as just
```rust
collect_into_array(IntoIter::new(self).chain(iter::repeat_with(f)))
```
but unfortunately that optimized very poorly.

As a result there's a bunch of refactoring in here to move the `Guard` struct that was previously private to `try_collect_into_array` over to its own module.  It's still very much internal -- it's visible only to the `core::array` module, with no intention of making this the anointed public thing.
@scottmcm scottmcm changed the title Add repeat and resize(_with) for arrays Add resize(_with) and (try_)repeat(_with) for arrays Dec 7, 2021
@scottmcm
Copy link
Member Author

scottmcm commented Dec 7, 2021

I added array::repeat_with and array::try_repeat_with to this PR. It's not obvious that we should stabilize both array::from_fn and array::repeat_with, but I think trying them both in nightly is reasonable. And maybe both would be fine -- after all, iter::repeat_with and iter::from_fn both exist. (For more, see #89379 (comment).)

@scottmcm
Copy link
Member Author

@m-ou-se Friendly ping for the new year.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 21, 2022

repeat for Copy types already exists as [expr; N], so I'm wondering if it should specifically mention clone in the name. And even for non-Copy types, [const { expr }; N] will work Soon™ for values like Vec::new(), which makes me wonder if there's many use cases for this function.

As you said, [try_]repeat_with is very close to [try_]from_fn. I'm not sure if it makes sense to have both. (And if we do end up having both, at least the documentation of each would have to very clearly refer to the other one.) The only advantage seems to be that one can write repeat_with(Vec::new) rather than from_fn(|_| Vec::new()). I'm not sure that outweighs the potential confusion of having two very similar functions.

For resize I'm worried about implying that arrays are resizable. Using this to turn a [i32; 2] into a [i32; 3] is fine, but 'resizing' a large array results in a lot of unnecssary copying. Do you have some example use cases? It makes me wonder if we should have a 'concat' function or some other interface instead, which is both more flexble and doesn't seem to imply it somehow modifies an existing array.

@scottmcm
Copy link
Member Author

scottmcm commented Feb 6, 2022

This is going to bit-rot soon from conflicts, so I'll just close it.

@scottmcm scottmcm closed this Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

4 participants