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::collect_array method #79659

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 73 additions & 0 deletions library/core/src/iter/traits/iterator.rs
Expand Up @@ -3,7 +3,9 @@
// can't split that into multiple files.

use crate::cmp::{self, Ordering};
use crate::mem::{self, MaybeUninit};
use crate::ops::{Add, ControlFlow, Try};
use crate::ptr;

use super::super::TrustedRandomAccess;
use super::super::{Chain, Cloned, Copied, Cycle, Enumerate, Filter, FilterMap, Fuse};
Expand Down Expand Up @@ -1670,6 +1672,77 @@ pub trait Iterator {
FromIterator::from_iter(self)
}

/// Collects all items from the iterator into an array of a specific size.
///
/// If the number of elements inside the iterator is exactly equal to the
/// array size, then the array is returned inside `Some`, otherwise `None`
/// is returned.
///
/// # Examples
///
/// ```
/// #![feature(iter_collect_array)]
///
/// let xs = [1, 2, 3];
/// let iter = xs.iter().copied();
/// let [_a, b, _c] = iter.clone().collect_array().unwrap();
/// assert_eq!(b, 2);
///
/// match iter.collect_array() {
/// Some([_, _]) => panic!("Didn't expect to see two only elements"),
/// None => (),
/// }
/// ```
#[inline]
#[unstable(reason = "new API", issue = "none", feature = "iter_collect_array")]
fn collect_array<const N: usize>(self) -> Option<[Self::Item; N]>
Copy link
Member

Choose a reason for hiding this comment

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

pondering: given that this will potentially collect only a prefix, should it be &mut self? The Option is enough to know whether the iterator was exhausted in the call, so one could use .collect_array::<4>() in a loop...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I think I misunderstood initially... You want to have "exact N" semantics, but you want &mut self at the same time?

This is interesting!

The problem I am seeing here is that, to get exactly N, you'll need to fetch N + 1 elements, so one would be dropped on the floor or stuffed into the Err variant somehow.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that it.collect_array::<3>() would be equivalent to try { [it.next()?, it.next()?, it.next()?] }. So you'd get None if the length is shorter than the number requested, but if there's more you'd be able to call again to get more of them.

(Maybe with a rename like sfackler mentions -- this is reminding me of as_chunks and array_chunks on slices, so maybe something like that?)

Copy link
Member

Choose a reason for hiding this comment

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

@scottmcm for iterators, self and &mut self is the same thing - you can always use .by_ref()

Copy link
Member

Choose a reason for hiding this comment

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

@WaffleLapkin Conceptually yes. But in implementation not exactly -- see all these benchmarks for example.

And if this doesn't exhaust the iterator, then it should be &mut self for consistency with other iterator methods that don't necessarily exhaust the iterator (like all and any, which are also &mut self).

where
Self: Sized,
{
struct Buf<T, const N: usize> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have this type somewhere already?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is one in [T; N]::map() method

Copy link
Member

Choose a reason for hiding this comment

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

Looks like there are more copies in other PRs, like #79451.

Maybe it'd be worth waiting for #75644? Then this might essentially just be [T; N]::try_generate(|_| it.next())...

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like we need core::collections::array_vec already :D

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
Member

Choose a reason for hiding this comment

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

I'm currently writing Iterator::array_chunks and I have +- the same code :)

(though it's a bit more convoluted since DoubleEndedItetator impl requires an ability to push_back)

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we rarely get to our needs decision items 🙃

If we keep tabs on these unwind-safe-partially-initialized-array types we're introducing I'd be happy not to block any of these PRs individually on them and run a pass over later to replace them with something more general.

// Safety invariant: first `len` items are initialized.
items: [MaybeUninit<T>; N],
len: usize,
}
impl<T, const N: usize> Buf<T, N> {
fn new() -> Buf<T, N> {
Buf { items: MaybeUninit::uninit_array(), len: 0 }
}
fn push(&mut self, item: T) -> Option<()> {
let slot = self.items.get_mut(self.len)?;
slot.write(item);
self.len += 1;
Some(())
}
fn into_array(mut self) -> Option<[T; N]> {
if self.len != N {
return None;
}
self.len = 0;
let res =
// SAFETY: `len` field invariant + the guard above.
unsafe { mem::transmute_copy::<[MaybeUninit<T>; N], [T; N]>(&self.items) };
Some(res)
}
}

impl<T, const N: usize> Drop for Buf<T, N> {
fn drop(&mut self) {
// SAFETY: `len` field invariant.
unsafe {
let slice = MaybeUninit::slice_assume_init_mut(&mut self.items[..self.len]);
ptr::drop_in_place(slice);
}
}
}

let mut buf: Buf<Self::Item, { N }> = Buf::new();
for elem in self {
buf.push(elem)?;
}
buf.into_array()
}

/// Consumes an iterator, creating two collections from it.
///
/// The predicate passed to `partition()` can return `true`, or `false`.
Expand Down