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::intersperse #75784

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions library/core/src/iter/adapters/intersperse.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
use super::Peekable;

/// An iterator adapter that places a separator between all elements.
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
/// An iterator adapter that places a separator between all elements.
/// An iterator adapter that places a separator between all elements.
///
/// This `struct` is created by [`Iterator::intersperse`]. See its
/// documentation for more.

Document how was in created.

#[unstable(feature = "iter_intersperse", reason = "recently added", issue = "none")]
#[derive(Debug, Clone)]
pub struct Intersperse<I: Iterator>
where
I::Item: Clone,
{
separator: I::Item,
iter: Peekable<I>,
Comment on lines +10 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe iter should comes first since it is the most important thing?

Suggested change
separator: I::Item,
iter: Peekable<I>,
iter: Peekable<I>,
separator: I::Item,

needs_sep: bool,
}

impl<I: Iterator> Intersperse<I>
where
I::Item: Clone,
{
pub(in crate::iter) fn new(iter: I, separator: I::Item) -> Self {
Self { iter: iter.peekable(), separator, needs_sep: false }
}
}

#[unstable(feature = "iter_intersperse", reason = "recently added", issue = "none")]
impl<I> Iterator for Intersperse<I>
where
I: Iterator,
I::Item: Clone,
{
type Item = I::Item;

#[inline]
fn next(&mut self) -> Option<I::Item> {
if self.needs_sep && self.iter.peek().is_some() {
self.needs_sep = false;
Some(self.separator.clone())
} else {
jonas-schievink marked this conversation as resolved.
Show resolved Hide resolved
self.needs_sep = true;
self.iter.next()
}
}
jonas-schievink marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Contributor

@pickfire pickfire Aug 23, 2020

Choose a reason for hiding this comment

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

Should we have a version of try_fold?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, hey, I wrote this! Specializing try_fold is probably not necessary. Iterator::for_each is implemented in terms of Iterator::fold, not Iterator::try_fold, so specializing fold alone is sufficient to get really substantial performance improvements.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I see that other specializations do include try_fold too. I think there may be a reason they do it.

Copy link
Member

Choose a reason for hiding this comment

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

There are several forwarding hierarchies in Iterator. fold and try_fold are separate ones.
See the (now somewhat outdated) analysis in the PR that made fold independent from try_fold: #72139 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@the8472 So it means we should implement all the important ones?

Copy link
Member

Choose a reason for hiding this comment

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

If the custom fold implementation provides a speedup over the default one which relies on next then the other ones would likely benefit too.
Additionally the other optimization and extension traits that most iterator adapters implement such as ExactSizeIterator, TrustedLen, DoubleEndedIterator, FusedIterator, TrustedRandomAccess all seem applicable here.

Copy link
Contributor Author

@jonas-schievink jonas-schievink Sep 6, 2020

Choose a reason for hiding this comment

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

Added an implementation of try_fold

Copy link
Member

Choose a reason for hiding this comment

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

You have removed try_fold again, but not the tests. Any reason for that?

fn fold<B, F>(mut self, init: B, mut f: F) -> B
where
Self: Sized,
F: FnMut(B, Self::Item) -> B,
{
let mut accum = init;

// Use `peek()` first to avoid calling `next()` on an empty iterator.
Copy link
Member

Choose a reason for hiding this comment

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

That's only useful for iterators that are not fused, right? Or what do we gain by not calling next() on an empty iterator?

Also, I think this would break for the following case:

// Imagine `it` is a non fused iterator that yields: `None`, `Some('a')`, `None`
it.intersperse('x').fold(String::new(), |mut s, c| { s.push(c); s })

That would result in "xa" with this implementation, but should yield "", right?

if !self.needs_sep || self.iter.peek().is_some() {
if let Some(x) = self.iter.next() {
accum = f(accum, x);
}
}
Comment on lines +50 to +55
Copy link
Contributor

@pickfire pickfire Sep 12, 2020

Choose a reason for hiding this comment

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

Wouldn't this result in sep, item, sep, item if next() was already called once before fold on iterator with three item?

I thought fold should always start with an item instead of a sep?

Copy link
Member

Choose a reason for hiding this comment

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

I thought fold should always start with an item instead of a sep?

Why would that be? As far as I see it, that's intended behavior. The iterator is just a sequence of items and if you already got the first one, methods like fold will start with the second item (which, in this case, is a separator).


let element = &self.separator;

self.iter.fold(accum, |mut accum, x| {
accum = f(accum, element.clone());
accum = f(accum, x);
accum
})
LukasKalbertodt marked this conversation as resolved.
Show resolved Hide resolved
}

fn size_hint(&self) -> (usize, Option<usize>) {
let (lo, hi) = self.iter.size_hint();
let next_is_elem = !self.needs_sep;
let lo = lo.saturating_sub(next_is_elem as usize).saturating_add(lo);
let hi = match hi {
Some(hi) => hi.saturating_sub(next_is_elem as usize).checked_add(hi),
None => None,
};
Comment on lines +68 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better?

Suggested change
let next_is_elem = !self.needs_sep;
let lo = lo.saturating_sub(next_is_elem as usize).saturating_add(lo);
let hi = match hi {
Some(hi) => hi.saturating_sub(next_is_elem as usize).checked_add(hi),
None => None,
};
let next_is_elem = !self.needs_sep as usize;
let lo = lo.saturating_sub(next_is_elem).saturating_add(lo);
let hi = match hi {
Some(hi) => hi.saturating_sub(next_is_elem).checked_add(hi),
None => None,
};

Comment on lines +70 to +73
Copy link
Member

Choose a reason for hiding this comment

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

No hard opinion, but this should work as well:

Suggested change
let hi = match hi {
Some(hi) => hi.saturating_sub(next_is_elem as usize).checked_add(hi),
None => None,
};
let hi = hi.and_then(|hi| hi.saturating_sub(next_is_elem as usize).checked_add(hi));

(lo, hi)
}
}
3 changes: 3 additions & 0 deletions library/core/src/iter/adapters/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@ use super::{
mod chain;
mod flatten;
mod fuse;
mod intersperse;
mod zip;

pub use self::chain::Chain;
#[stable(feature = "rust1", since = "1.0.0")]
pub use self::flatten::{FlatMap, Flatten};
pub use self::fuse::Fuse;
#[unstable(feature = "iter_intersperse", reason = "recently added", issue = "none")]
pub use self::intersperse::Intersperse;
use self::zip::try_get_unchecked;
#[unstable(feature = "trusted_random_access", issue = "none")]
pub use self::zip::TrustedRandomAccess;
Expand Down
3 changes: 2 additions & 1 deletion library/core/src/iter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,8 @@ pub use self::adapters::Cloned;
pub use self::adapters::Copied;
#[stable(feature = "iterator_flatten", since = "1.29.0")]
pub use self::adapters::Flatten;

#[unstable(feature = "iter_intersperse", reason = "recently added", issue = "none")]
pub use self::adapters::Intersperse;
#[unstable(feature = "iter_map_while", reason = "recently added", issue = "68537")]
pub use self::adapters::MapWhile;
#[unstable(issue = "none", feature = "inplace_iteration")]
Expand Down
24 changes: 23 additions & 1 deletion library/core/src/iter/traits/iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::ops::{Add, ControlFlow, Try};
use super::super::TrustedRandomAccess;
use super::super::{Chain, Cloned, Copied, Cycle, Enumerate, Filter, FilterMap, Fuse};
use super::super::{FlatMap, Flatten};
use super::super::{FromIterator, Product, Sum, Zip};
use super::super::{FromIterator, Intersperse, Product, Sum, Zip};
use super::super::{
Inspect, Map, MapWhile, Peekable, Rev, Scan, Skip, SkipWhile, StepBy, Take, TakeWhile,
};
Expand Down Expand Up @@ -536,6 +536,28 @@ pub trait Iterator {
Zip::new(self, other.into_iter())
}

/// Places a copy of `separator` between all elements.
jonas-schievink marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Examples
///
/// Basic usage:
///
/// ```
/// #![feature(iter_intersperse)]
///
/// let hello = ["Hello", "World"].iter().copied().intersperse(" ").collect::<String>();
/// assert_eq!(hello, "Hello World");
Comment on lines +548 to +549
Copy link
Contributor

Choose a reason for hiding this comment

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

Add different example and some humor (not tested)

Suggested change
/// let hello = ["Hello", "World"].iter().copied().intersperse(" ").collect::<String>();
/// assert_eq!(hello, "Hello World");
/// let s = ["996", "icu"].iter().copied().intersperse(".").collect::<String>();
/// assert_eq!(s, "996.icu");
///
/// let v = [&[9, 9, 6], &[6, 6, 9], &[1, 1, 2]];
/// let v = v.iter().copied().intersperse(&[0]).collect::<Vec<_>>();
/// assert_eq!(v, &[9, 9, 6, 0, 6, 6, 9, 0, 1, 1, 2]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, how about the second example for &[u8]? At least we don't just show &str.

Copy link
Member

Choose a reason for hiding this comment

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

Another example where separator is yielded more than once would be nice. And preferably with slices instead of strings.

/// ```
#[inline]
#[unstable(feature = "iter_intersperse", reason = "recently added", issue = "none")]
fn intersperse(self, separator: Self::Item) -> Intersperse<Self>
where
Self: Sized,
Self::Item: Clone,
{
Intersperse::new(self, separator)
}

/// Takes a closure and creates an iterator which calls that closure on each
/// element.
///
Expand Down
82 changes: 82 additions & 0 deletions library/core/tests/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3222,3 +3222,85 @@ fn test_flatten_non_fused_inner() {
assert_eq!(iter.next(), Some(1));
assert_eq!(iter.next(), None);
}

#[test]
fn test_intersperse() {
let xs = ["a", "", "b", "c"];
let v: Vec<&str> = xs.iter().map(|x| x.clone()).intersperse(", ").collect();
Copy link
Contributor

@pickfire pickfire Sep 12, 2020

Choose a reason for hiding this comment

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

Suggested change
let v: Vec<&str> = xs.iter().map(|x| x.clone()).intersperse(", ").collect();
let v: Vec<_> = xs.iter().copied().intersperse(", ").collect();

let text: String = v.concat();
assert_eq!(text, "a, , b, c".to_string());

let ys = [0, 1, 2, 3];
let mut it = ys[..0].iter().map(|x| *x).intersperse(1);
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 it = ys[..0].iter().map(|x| *x).intersperse(1);
let mut it = ys[..0].iter().copied().intersperse(1);

// copied is the same as .map(|&x| x)

Not sure if this works but probably it does.

assert!(it.next() == None);
}

#[test]
fn test_intersperse_size_hint() {
let xs = ["a", "", "b", "c"];
let mut iter = xs.iter().map(|x| x.clone()).intersperse(", ");
assert_eq!(iter.size_hint(), (7, Some(7)));

assert_eq!(iter.next(), Some("a"));
assert_eq!(iter.size_hint(), (6, Some(6)));
assert_eq!(iter.next(), Some(", "));
assert_eq!(iter.size_hint(), (5, Some(5)));

assert_eq!([].iter().intersperse(&()).size_hint(), (0, Some(0)));
Copy link
Contributor

@pickfire pickfire Sep 12, 2020

Choose a reason for hiding this comment

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

Should we add a test for one without upper bound? I think this is sufficient but just wondering if we need it since someone could change None to return Some(T).

}

#[test]
fn test_fold_specialization_intersperse() {
let mut iter = (1..2).intersperse(0);
iter.clone().for_each(|x| assert_eq!(Some(x), iter.next()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test for fold and try_fold after one item is consumed?


let mut iter = (1..3).intersperse(0);
iter.clone().for_each(|x| assert_eq!(Some(x), iter.next()));

let mut iter = (1..4).intersperse(0);
iter.clone().for_each(|x| assert_eq!(Some(x), iter.next()));
Comment on lines +3257 to +3261
Copy link
Contributor

@pickfire pickfire Sep 12, 2020

Choose a reason for hiding this comment

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

I don't understand what is the difference between these and the above test? Do we need these?

}

#[test]
fn test_try_fold_specialization_intersperse_ok() {
let mut iter = (1..2).intersperse(0);
iter.clone().try_for_each(|x| {
assert_eq!(Some(x), iter.next());
Some(())
});

let mut iter = (1..3).intersperse(0);
iter.clone().try_for_each(|x| {
assert_eq!(Some(x), iter.next());
Some(())
});

let mut iter = (1..4).intersperse(0);
iter.clone().try_for_each(|x| {
assert_eq!(Some(x), iter.next());
Some(())
});
}

#[test]
fn test_try_fold_specialization_intersperse_err() {
let orig_iter = ["a", "b"].iter().copied().intersperse("-");

// Abort after the first item.
let mut iter = orig_iter.clone();
iter.try_for_each(|_| None::<()>);
assert_eq!(iter.next(), Some("-"));
assert_eq!(iter.next(), Some("b"));
assert_eq!(iter.next(), None);

// Abort after the second item.
let mut iter = orig_iter.clone();
iter.try_for_each(|item| if item == "-" { None } else { Some(()) });
assert_eq!(iter.next(), Some("b"));
assert_eq!(iter.next(), None);

// Abort after the third item.
let mut iter = orig_iter.clone();
iter.try_for_each(|item| if item == "b" { None } else { Some(()) });
assert_eq!(iter.next(), None);
}
1 change: 1 addition & 0 deletions library/core/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#![feature(int_error_matching)]
#![feature(array_value_iter)]
#![feature(iter_partition_in_place)]
#![feature(iter_intersperse)]
#![feature(iter_is_partitioned)]
#![feature(iter_order_by)]
#![feature(cmp_min_max_by)]
Expand Down
1 change: 0 additions & 1 deletion src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use crate::clean::{
};
use crate::core::DocContext;

use itertools::Itertools;
use rustc_data_structures::fx::FxHashSet;
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
Expand Down
1 change: 1 addition & 0 deletions src/librustdoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#![feature(crate_visibility_modifier)]
#![feature(never_type)]
#![feature(once_cell)]
#![feature(iter_intersperse)]
#![recursion_limit = "256"]

#[macro_use]
Expand Down