Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
36 changes: 35 additions & 1 deletion src/libcore/iter/iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use cmp::Ordering;

use super::{Chain, Cycle, Cloned, Enumerate, Filter, FilterMap, FlatMap, Fuse};
use super::{Inspect, Map, Peekable, Scan, Skip, SkipWhile, Take, TakeWhile, Rev};
use super::{Inspect, Map, Peekable, Scan, Skip, SkipWhile, StepBy, Take, TakeWhile, Rev};
use super::{Zip, Sum, Product};
use super::{ChainState, FromIterator, ZipImpl};

Expand Down Expand Up @@ -258,6 +258,40 @@ pub trait Iterator {
None
}

/// Creates an iterator starting at the same point, but stepping by
/// the given amount at each iteration.
///
/// Note that it will always return the first element of the range,
/// regardless of the step given.
///
/// # Panics
///
/// If the given step is `0`, the method will panic if debug assertions are
/// enabled.
Copy link
Member

Choose a reason for hiding this comment

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

nit: This comment makes me think debug_assert!, but it's not. Does it need more than "This method will panic if the given step is 0."?

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 just looked at how the other methods in Iterator that panic were phrased and tried doing something similar for consistency.

I just checked now, and I didn't know of debug_assert!, which is what the other methods are using. I've been mislead! :P I'll fix it soon. I also have to figure out what to write on the Unstable Book to make that tidy check pass 🙄

///
/// # Examples
///
/// Basic usage:
///
/// ```
/// #![feature(iterator_step_by)]
/// let a = [0, 1, 2, 3, 4, 5];
/// let mut iter = a.into_iter().step_by(2);
///
/// assert_eq!(iter.next(), Some(&0));
/// assert_eq!(iter.next(), Some(&2));
/// assert_eq!(iter.next(), Some(&4));
/// assert_eq!(iter.next(), None);
/// ```
#[inline]
#[unstable(feature = "iterator_step_by",
reason = "unstable replacement of Range::step_by",
issue = "27741")]
fn step_by(self, step: usize) -> StepBy<Self> where Self: Sized {
assert!(step != 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I'd expect this method to panic, especially if I'm using it programmatically. I'm in no position to define this, but: If this assert stays, I'd add a custom message.

Copy link
Member

Choose a reason for hiding this comment

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

I would personally expect the behaviour of .peek() for every_nth(0).next(). Troublesome to implement though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nagisa more like impossible without cloning (peak returns a reference, next would return a value).

From the description ("skipping step elements at a time"), I'd expect every_nth(0) to return every item and every_nth(1) to skip every other item. This would also be consistent with nth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it would be best to have that behavior instead. I'll make the changes now.

StepBy{iter: self, step: step - 1, first_take: true}
}

/// Takes two iterators and creates a new iterator over both in sequence.
///
/// `chain()` will return a new iterator which will first iterate over
Expand Down
37 changes: 36 additions & 1 deletion src/libcore/iter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ pub use self::iterator::Iterator;
pub use self::range::Step;
#[unstable(feature = "step_by", reason = "recent addition",
issue = "27741")]
pub use self::range::StepBy;
pub use self::range::StepBy as DeprecatedStepBy;

#[stable(feature = "rust1", since = "1.0.0")]
pub use self::sources::{Repeat, repeat};
Expand Down Expand Up @@ -520,6 +520,41 @@ impl<I> Iterator for Cycle<I> where I: Clone + Iterator {
#[unstable(feature = "fused", issue = "35602")]
impl<I> FusedIterator for Cycle<I> where I: Clone + Iterator {}

/// An iterator that steps by n elements every iteration.
///
/// This `struct` is created by the [`step_by`] method on [`Iterator`]. See
/// its documentation for more.
///
/// [`step_by`]: trait.Iterator.html#method.step_by
/// [`Iterator`]: trait.Iterator.html
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
#[unstable(feature = "iterator_step_by",
reason = "unstable replacement of Range::step_by",
issue = "27741")]
#[derive(Clone, Debug)]
pub struct StepBy<I> {
iter: I,
step: usize,
first_take: bool,
}

#[unstable(feature = "iterator_step_by",
reason = "unstable replacement of Range::step_by",
issue = "27741")]
impl<I> Iterator for StepBy<I> where I: Iterator {
type Item = I::Item;

#[inline]
fn next(&mut self) -> Option<Self::Item> {
if self.first_take {
self.first_take = false;
self.iter.next()
} else {
self.iter.nth(self.step)
}
}
}

/// An iterator that strings two iterators together.
///
/// This `struct` is created by the [`chain`] method on [`Iterator`]. See its
Expand Down
29 changes: 28 additions & 1 deletion src/libcore/tests/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,33 @@ fn test_iterator_chain_find() {
assert_eq!(iter.next(), None);
}

#[test]
fn test_iterator_step_by() {
// Identity
// Replace with (0..).step_by(1) after Range::step_by gets removed
let mut it = Iterator::step_by((0..), 1).take(3);
assert_eq!(it.next(), Some(0));
assert_eq!(it.next(), Some(1));
assert_eq!(it.next(), Some(2));
assert_eq!(it.next(), None);

// Replace with (0..).step_by(3) after Range::step_by gets removed
let mut it = Iterator::step_by((0..), 3).take(4);
assert_eq!(it.next(), Some(0));
assert_eq!(it.next(), Some(3));
assert_eq!(it.next(), Some(6));
assert_eq!(it.next(), Some(9));
assert_eq!(it.next(), None);
}

#[test]
#[should_panic]
fn test_iterator_step_by_zero() {
// Replace with (0..).step_by(0) after Range::step_by gets removed
let mut it = Iterator::step_by((0..), 0);
it.next();
}

#[test]
fn test_filter_map() {
let it = (0..).step_by(1).take(10)
Expand Down Expand Up @@ -1119,4 +1146,4 @@ fn test_step_replace_no_between() {
let y = x.replace_one();
assert_eq!(x, 1);
assert_eq!(y, 5);
}
}
1 change: 1 addition & 0 deletions src/libcore/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#![feature(fixed_size_array)]
#![feature(flt2dec)]
#![feature(fmt_internals)]
#![feature(iterator_step_by)]
#![feature(i128_type)]
#![feature(iter_rfind)]
#![feature(libc)]
Expand Down