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 Iterator::array_windows() #92394

Closed

Conversation

rossmacarthur
Copy link
Contributor

@rossmacarthur rossmacarthur commented Dec 29, 2021

This has been similarly implemented as .tuple_windows() in itertools as Itertools::tuple_windows(). But it makes more sense with arrays since all elements are the same type.

I will update stability attributes with a tracking issue if accepted.

See also #92393

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 29, 2021
/// ```
/// #![feature(iter_array_windows)]
///
/// let mut iter = "rust".chars().array_windows();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// let mut iter = "rust".chars().array_windows();
/// let mut iter = "rust".chars().array_windows::<2>();

I think you missed it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not required because of type inference

@the8472
Copy link
Member

the8472 commented Dec 29, 2021

This involves a lot of clone and rotate_left operations, so this has significantly different performance characteristics compared to [T]::array_windows. Since clone operations can be arbitrarily expensive offering something like this under the same name as a fairly cheap method could be misleading if someone tried to substitute one for the other.

@rossmacarthur
Copy link
Contributor Author

Thanks @the8472, you make some good points. I do think in a lot of cases it might be more performant to collect into a Vec and then use [T]::array_windows, so you think this API is ever something we would want add to Iterator even under a different name?

@the8472
Copy link
Member

the8472 commented Jan 1, 2022

Perhaps reviving #82413 would be a better option since the map closure can take a reference to the internal iterator state and thus incur no cloning overhead. Additionally it amortizes the rotation cost by increasing the internal buffer size.

{
#[inline]
pub(in crate::iter) fn new(iter: I) -> Self {
assert!(N != 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some way to assert it in compile time?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be ideal but I couldn't get it to work. I tried it for #92393 and get the following error

error[E0401]: can't use generic parameters from outer function
  --> library/core/src/iter/adapters/array_chunks.rs:86:31
   |
81 | impl<I, const N: usize> ArrayChunks<I, N>
   |               - const parameter from outer function
...
86 |         const _: () = assert!(N != 0, "chunk size must be non-zero");
   |                               ^ use of generic parameter from outer function

For more information about this error, try `rustc --explain E0401`.

And if I make the const part of the impl block it doesn't result in a compile time error.

@rossmacarthur
Copy link
Contributor Author

Based on the discussion above I'm closing this. I might look at reviving #82413 instead.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants