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

Tracking Issue for Iterator::array_chunks #100450

Open
1 of 7 tasks
WaffleLapkin opened this issue Aug 12, 2022 · 7 comments
Open
1 of 7 tasks

Tracking Issue for Iterator::array_chunks #100450

WaffleLapkin opened this issue Aug 12, 2022 · 7 comments
Labels
A-array Area: `[T; N]` C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@WaffleLapkin
Copy link
Member

WaffleLapkin commented Aug 12, 2022

Feature gate: #![feature(iter_array_chunks)]

This is a tracking issue for Iterator::array_chunks, an iterator adapter that allows to group elements of an iterator.

Public API

// trait Iterator

fn array_chunks<const N: usize>(self) -> ArrayChunks<Self, N>
where
    Self: Sized,self);
// core::iter

pub struct ArrayChunks<I: Iterator, const N: usize> { ... }

impl<I, const N: usize> ArrayChunks<I, N> {
    pub fn into_remainder(self) -> Option<array::IntoIter<I::Item, N>>;
}

impl<I: Iterator, const N: usize> Iterator for ArrayChunks<I, N> {
    type Item = [I::Item; N];
}

impl<I: DoubleEndedIterator + ExactSizeIterator, const N: usize> DoubleEndedIterator for ArrayChunks<I, N> {}
impl<I: FusedIterator, const N: usize> FusedIterator for ArrayChunks<I, N> {}
impl<I: ExactSizeIterator, const N: usize> ExactSizeIterator for ArrayChunks<I, N> {}

Steps / History

Unresolved Questions

  • Should DoubleEndedIterator be implemented?
    • Is there a use-case for this?
    • In order to yield the same items either way we require ExactSizeIterator
    • Note that .rev().array_chunks() and .array_chunks().rev() are different
    • If we decide to keep DoubleEndedIterator implementation should be improved
      • For example, maybe there should be Iterator::next_chunk_back like there is for forward
    • Is the unwrap_err_unchecked in next_back_remainder sound, despite not using TrustedLen? (I didn't block the original PR on this because it goes through take, which I think we can trust, but I'd like someone smarter than me to think about it and confirm.)
  • Should this be a type error for N != 0? (See also this same question in [T]::as_chunks.)
  • What's the tradeoff between the item type being array::IntoIter<T, N> and the current one where the items are [T; N] and there's a remainder? ([T]::array_chunks has this question too and is unstable.)

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@WaffleLapkin WaffleLapkin added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 12, 2022
@the8472
Copy link
Member

the8472 commented Aug 13, 2022

What's the tradeoff between the item type being array::IntoIter<T, N> and the current one where the items are [T; N] and there's a remainder?

It is definitely valuable to have a guaranteed, fixed length in the happy path. It aids vectorization and known-length specialization. It could even help in-place iteration in shapes like vec.into_iter().array_chunks().map(...).flatten().collect().

@rossmacarthur
Copy link
Contributor

rossmacarthur commented Nov 1, 2022

Might be good to align the naming of this function with Iterator::next_chunk, i.e. either

  • Iterator::chunks
  • Iterator::next_chunk

Or

  • Iterator::array_chunks
  • Iterator::next_array_chunk (or Iterator::next_array)?

@camelid
Copy link
Member

camelid commented Nov 20, 2022

I think I'd vote for Iterator::chunks since arrays are a pretty clear fit for what this function accomplishes and array_chunks is a bit unwieldy.

@adamse
Copy link
Contributor

adamse commented Dec 3, 2022

It seems good to follow the same naming conventions as slice::array_chunks #74985 where possible?

@andria-dev
Copy link

andria-dev commented Dec 12, 2023

I think that the name Iterator::array_chunks is fine, but not great. The simple present tense of the verb 'to chunk' (i.e. Iterator::chunk) could better indicate the action portion of organizing separate smaller units into single larger units. This should also avoid overriding the meaning of the plural noun 'chunks' as defined by Itertools::chunks (#104969 (comment)). As for following the naming convention of slice::array_chunks (#74985), it is also a nightly-only experimental API, so now should be a perfect time to change both as the above reasoning for using chunk applies there as well.

@GnomedDev
Copy link
Contributor

An interesting unresolved question was brought up in #116000, should ArrayChunks::into_remainder return Option?

@ronnodas
Copy link

I do not see the point of artificially converting an empty slice into a None, which needs to be unwrapped on use. Or semantically, it's not that the remainder is undefined, it just happens to be empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: `[T; N]` C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants