Skip to content

Add Interleave iterator #15886

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

Closed
wants to merge 1 commit 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
127 changes: 126 additions & 1 deletion src/libcore/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ use clone::Clone;
use cmp;
use cmp::{PartialEq, PartialOrd, Ord};
use mem;
use num::{Zero, One, CheckedAdd, CheckedSub, Saturating, ToPrimitive, Int};
use num::{Zero, One, CheckedAdd, CheckedSub, CheckedMul, Saturating, ToPrimitive, Int};
use ops::{Add, Mul, Sub};
use option::{Option, Some, None};
use uint;
Expand Down Expand Up @@ -144,6 +144,25 @@ pub trait Iterator<A> {
Zip{a: self, b: other}
}

/// Creates an iterator which interleaves the contents of this and the specified
/// iterator.
///
/// # Example
///
/// ```rust
/// let a = [0i, 2i];
/// let b = [1i];
/// let mut it = a.iter().interleave(b.iter());
/// assert_eq!(it.next().unwrap(), &0i);
/// assert_eq!(it.next().unwrap(), &1i);
/// assert_eq!(it.next().unwrap(), &1i);
/// assert!(it.next().is_none());
/// ```
#[inline]
fn interleave<U: Iterator<A>>(self, other: U) -> Interleave<Self, U> {
Interleave{a: self, b: other, using_first: true}
}

/// Creates a new iterator which will apply the specified function to each
/// element returned by the first, yielding the mapped element instead.
///
Expand Down Expand Up @@ -1241,6 +1260,112 @@ RandomAccessIterator<(A, B)> for Zip<T, U> {
}
}

/// An iterator which interleaves the contents of two other iterators
#[deriving(Clone)]
pub struct Interleave<T, U> {
a: T,
b: U,
using_first: bool
}

fn interleave_len(a_len: uint, b_len: uint, using_first: bool) -> Option<uint> {
let off_by_one = using_first as uint;
if a_len < b_len {
a_len.checked_mul(&2).and_then(|x| x.checked_add(&(1 - off_by_one)))
} else if b_len < a_len {
b_len.checked_mul(&2).and_then(|x| x.checked_add(&off_by_one))
} else {
a_len.checked_mul(&2)
}
}

// Note to developers: sequence will always look like a,b,a,b,a,... but the last element
// to be yielded will be *either* an a or b. Thus we may yield one more a than b.
// This is a key fact to the design of Interleave.
impl<A, T: Iterator<A>, U: Iterator<A>> Iterator<A> for Interleave<T, U> {
#[inline]
fn next(&mut self) -> Option<A> {
let use_a = self.using_first;
self.using_first = !use_a;

if use_a {
self.a.next()
} else {
self.b.next()
}
}

#[inline]
fn size_hint(&self) -> (uint, Option<uint>) {
let (a_lower, a_upper) = self.a.size_hint();
let (b_lower, b_upper) = self.b.size_hint();

let lower = interleave_len(a_lower, b_lower, self.using_first).unwrap_or(uint::MAX);

let upper = match (a_upper, b_upper) {
(Some(x), Some(y)) => interleave_len(x, y, self.using_first),
// If one is missing, best we can do is assume it's huge
(Some(x), None) => interleave_len(x, uint::MAX, self.using_first),
(None, Some(y)) => interleave_len(uint::MAX, y, self.using_first),
(None, None) => None
};

(lower, upper)
}
}

impl<A, T: ExactSize<A>, U: ExactSize<A>> DoubleEndedIterator<A>
for Interleave<T, U> {
#[inline]
fn next_back(&mut self) -> Option<A> {
let (a_sz, a_upper) = self.a.size_hint();
let (b_sz, b_upper) = self.b.size_hint();
assert!(a_upper == Some(a_sz));
assert!(b_upper == Some(b_sz));
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably isn't the PR for it, but I'm surprised ExactSize doesn't just vend a method .exact_size() that includes the assert, so that way iterator adaptors don't have to keep asserting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly it struck me as weird to even bother with these asserts. Not trusting something that claims to implement an API to implement it strikes me as way over-defensive. But I figured if Zip does it, I probably should.

I could write up a PR to add it if you're honestly interested though, since I'm hacking away in here already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm not really sure what the correct answer is. I think the asserts are here because the ExactSize trait demands invariants that can be broken without unsafe code. So the idea is to fail early in any clients of ExactSize if the invariant is broken, so that way the client code doesn't break its own invariants. But on the other hand, it really does seem silly to have to constantly assert that a given trait obeys its own invariants.

Copy link
Member

Choose a reason for hiding this comment

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

kballard it effectively does now -- the .len() method; the Zip impl just hasn't been updated to use it.

I think being overly defensive is fine; I mean that was a way to introduce the ExactSize trait without too many detractors -- I just wanted a way to sensibly define a double ended enumerate() on vec iterator; I'm not sure if every type should bother doing the same


let off_by_one = self.using_first as uint;

if a_sz < b_sz {
for _ in range(1 - off_by_one, b_sz - a_sz) { self.b.next_back(); }
} else if a_sz > b_sz {
for _ in range(off_by_one, a_sz - b_sz) { self.a.next_back(); }
}
let (a_sz, _) = self.a.size_hint();
let (b_sz, _) = self.b.size_hint();
assert!(a_sz == b_sz || a_sz + (1 - off_by_one) == b_sz + off_by_one);
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is based on the Zip iterator code, but I'm not sure what the point is of re-fetching the size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kballard: to determine how much you've actually cut out, I guess? I mean, you can figure it out based on your range, in theory. This code seems very defensively designed. At least for my use, it just means less code spent tracking which branch I went into and edge-case interactions with almost-empty ranges.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could just make a_sz and b_sz mut and modify them as appropriate in the if-condition. It will be potentially much more efficient, as size_hint() may have to run an arbitrarily-complex calculation.

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'm tempted to leave this as part of the general "assert that everything still works" pattern that seems to be around the file. I agree that it's wasteful, but it's wasteful to have the asserts at all. I'd rather be consistent and either have both or neither. Disagree?

We should also update Zip to match, if we do anything.

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 it doesn't really matter. My preference would be to skip the asserts here, and let Zip be updated in a different PR (if anyone really cares), but if you want to leave the asserts in for consistency, I suppose you can do that.

Copy link
Member

Choose a reason for hiding this comment

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

It should probably be a debug_assert!(...).

Copy link
Contributor

Choose a reason for hiding this comment

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

@sfackler I wish. I think it's just assert!() in Zip (and therefore here) to ensure that user-defined iterators meet the invariants. The problem is, ideally that testing would occur if the user-defined iterator is compiled with debug asserts enabled, but using debug_assert!() here only runs the assert if the standard library is compiled with debug asserts enabled. Therefore, it just uses assert!().


if (self.using_first && a_sz > b_sz) || (!self.using_first && a_sz == b_sz) {
self.a.next_back()
} else {
self.b.next_back()
}
}
}

impl<A, T: RandomAccessIterator<A>, U: RandomAccessIterator<A>>
RandomAccessIterator<A> for Interleave<T, U> {
#[inline]
fn indexable(&self) -> uint {
interleave_len(self.a.indexable(), self.b.indexable(), self.using_first)
.unwrap_or(uint::MAX)
}

#[inline]
fn idx(&mut self, index: uint) -> Option<A> {
let off_by_one = self.using_first as uint;

if index < self.indexable() {
if index % 2 == (1 - off_by_one) {
self.a.idx((index - (1 - off_by_one)) / 2)
} else {
self.b.idx((index - off_by_one) / 2)
}
} else {
None
}
}
}

/// An iterator which maps the values of `iter` with `f`
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
pub struct Map<'a, A, B, T> {
Expand Down
40 changes: 40 additions & 0 deletions src/libcoretest/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ fn test_iterator_size_hint() {
assert_eq!(c.enumerate().size_hint(), (uint::MAX, None));
assert_eq!(c.chain(vi.map(|&i| i)).size_hint(), (uint::MAX, None));
assert_eq!(c.zip(vi).size_hint(), (10, Some(10)));
assert_eq!(c.iterleave(vi).size_hint(), (20, Some(21)))
assert_eq!(c.scan(0i, |_,_| Some(0i)).size_hint(), (0, None));
assert_eq!(c.filter(|_| false).size_hint(), (0, None));
assert_eq!(c.map(|_| 0i).size_hint(), (uint::MAX, None));
Expand All @@ -350,6 +351,7 @@ fn test_iterator_size_hint() {
assert_eq!(vi.enumerate().size_hint(), (10, Some(10)));
assert_eq!(vi.chain(v2.iter()).size_hint(), (13, Some(13)));
assert_eq!(vi.zip(v2.iter()).size_hint(), (3, Some(3)));
assert_eq!(vi.interleave(v2.iter()).size_hint(), (7, Some(7)));
assert_eq!(vi.scan(0i, |_,_| Some(0i)).size_hint(), (0, Some(10)));
assert_eq!(vi.filter(|_| false).size_hint(), (0, Some(10)));
assert_eq!(vi.map(|i| i+1).size_hint(), (10, Some(10)));
Expand Down Expand Up @@ -476,6 +478,23 @@ fn test_double_ended_zip() {
assert_eq!(it.next(), None);
}

#[test]
fn test_double_ended_interleave() {
let xs = [1i, 3, 5, 7, 8];
let ys = [2i, 4, 6];
let a = xs.iter().map(|&x| x);
let b = ys.iter().map(|&x| x);
let mut it = a.interleave(b);
assert_eq!(it.next(), Some(1));
assert_eq!(it.next(), Some(2));
assert_eq!(it.next_back(), Some(7));
assert_eq!(it.next_back(), Some(6));
assert_eq!(it.next(), Some(3));
assert_eq!(it.next_back(), Some(5));
assert_eq!(it.next_back(), Some(4));
assert_eq!(it.next(), None);
}

#[test]
fn test_double_ended_filter() {
let xs = [1i, 2, 3, 4, 5, 6];
Expand Down Expand Up @@ -621,6 +640,27 @@ fn test_random_access_zip() {
check_randacc_iter(xs.iter().zip(ys.iter()), cmp::min(xs.len(), ys.len()));
}

#[test]
fn test_random_access_interleave() {
let xs = [1i, 3, 5, 7, 8]; // 8 should not be accessible
let ys = [2i, 4, 6];
let mut it = xs.iter().interleave(ys.iter());
assert_eq!(it.idx(0).unwrap(), &1);
assert_eq!(it.idx(3).unwrap(), &4);
assert_eq!(it.idx(6).unwrap(), &7);
assert!(it.idx(7).is_none());

it.next();
it.next();
it.next_back();

assert_eq!(it.idx(0).unwrap(), &3);
assert_eq!(it.idx(3).unwrap(), &7);
assert!(it.idx(4).is_none());

check_randacc_iter(it, xs.len() + ys.len() - 3);
}

#[test]
fn test_random_access_take() {
let xs = [1i, 2, 3, 4, 5];
Expand Down