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 Range[Inclusive]::is_empty #48087

Merged
merged 5 commits into from
Feb 15, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion src/libcore/iter/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ pub trait ExactSizeIterator: Iterator {
/// ```
/// #![feature(exact_size_is_empty)]
///
/// let mut one_element = 0..1;
/// let mut one_element = std::iter::once(0);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this related to this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because using 0..1 means the code wasn't actually using ExactSizeIterator; the inherent method was getting picked up instead. For normal code that's not a problem--they do exactly the same thing--but I figured the doctest should actually use the method it's documenting.

/// assert!(!one_element.is_empty());
///
/// assert_eq!(one_element.next(), Some(0));
Expand Down
54 changes: 52 additions & 2 deletions src/libcore/ops/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ impl<Idx: fmt::Debug> fmt::Debug for Range<Idx> {
}
}

#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
impl<Idx: PartialOrd<Idx>> Range<Idx> {
/// Returns `true` if `item` is contained in the range.
///
Expand All @@ -109,9 +108,26 @@ impl<Idx: PartialOrd<Idx>> Range<Idx> {
/// assert!(!(3..3).contains(3));
/// assert!(!(3..2).contains(3));
/// ```
#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
pub fn contains(&self, item: Idx) -> bool {
(self.start <= item) && (item < self.end)
}

/// Returns `true` if the range contains no items.
///
/// # Examples
///
/// ```
/// #![feature(range_is_empty)]
///
/// assert!(!(3..5).is_empty());
/// assert!( (3..3).is_empty());
/// assert!( (3..2).is_empty());
/// ```
#[unstable(feature = "range_is_empty", reason = "recently added", issue = "123456789")]
pub fn is_empty(&self) -> bool {
!(self.start < self.end)
}
}

/// A range only bounded inclusively below (`start..`).
Expand Down Expand Up @@ -246,6 +262,13 @@ impl<Idx: PartialOrd<Idx>> RangeTo<Idx> {
/// The `RangeInclusive` `start..=end` contains all values with `x >= start`
/// and `x <= end`.
///
/// This iterator is [fused], but the specific values of `start` and `end` after
/// iteration has finished are **unspecified** other than that [`.is_empty()`]
/// will return `true` once no more values will be produced.
///
/// [fused]: ../iter/trait.FusedIterator.html
/// [`.is_empty()`]: #method.is_empty
///
/// # Examples
///
/// ```
Expand Down Expand Up @@ -280,7 +303,6 @@ impl<Idx: fmt::Debug> fmt::Debug for RangeInclusive<Idx> {
}
}

#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
/// Returns `true` if `item` is contained in the range.
///
Expand All @@ -298,9 +320,37 @@ impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
/// assert!( (3..=3).contains(3));
/// assert!(!(3..=2).contains(3));
/// ```
#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
pub fn contains(&self, item: Idx) -> bool {
self.start <= item && item <= self.end
}

/// Returns `true` if the range contains no items.
///
/// # Examples
///
/// ```
/// #![feature(range_is_empty,inclusive_range_syntax)]
///
/// assert!(!(3..=5).is_empty());
/// assert!(!(3..=3).is_empty());
/// assert!( (3..=2).is_empty());
/// ```
///
/// This method returns `true` after iteration has finished:
///
/// ```
/// #![feature(range_is_empty,inclusive_range_syntax)]
///
/// let mut r = 3..=5;
/// for _ in r.by_ref() {}
/// // Precise field values are unspecified here
/// assert!(r.is_empty());
/// ```
#[unstable(feature = "range_is_empty", reason = "recently added", issue = "123456789")]
pub fn is_empty(&self) -> bool {
!(self.start <= self.end)
}
}

/// A range only bounded inclusively above (`..=end`).
Expand Down
61 changes: 52 additions & 9 deletions src/libcore/tests/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1322,42 +1322,84 @@ fn test_range() {
(isize::MAX as usize + 2, Some(isize::MAX as usize + 2)));
}

#[test]
fn test_range_exhaustion() {
let mut r = 10..10;
assert!(r.is_empty());
assert_eq!(r.next(), None);
assert_eq!(r.next_back(), None);
assert_eq!(r, 10..10);

let mut r = 10..12;
assert_eq!(r.next(), Some(10));
assert_eq!(r.next(), Some(11));
assert!(r.is_empty());
assert_eq!(r, 12..12);
assert_eq!(r.next(), None);

let mut r = 10..12;
assert_eq!(r.next_back(), Some(11));
assert_eq!(r.next_back(), Some(10));
assert!(r.is_empty());
assert_eq!(r, 10..10);
assert_eq!(r.next_back(), None);

let mut r = 100..10;
assert!(r.is_empty());
assert_eq!(r.next(), None);
assert_eq!(r.next_back(), None);
assert_eq!(r, 100..10);
}

#[test]
fn test_range_inclusive_exhaustion() {
let mut r = 10..=10;
assert_eq!(r.next(), Some(10));
assert_eq!(r, 1..=0);
assert!(r.is_empty());
assert_eq!(r.next(), None);
assert_eq!(r.next(), None);

let mut r = 10..=10;
assert_eq!(r.next_back(), Some(10));
assert_eq!(r, 1..=0);
assert!(r.is_empty());
assert_eq!(r.next_back(), None);

let mut r = 10..=12;
assert_eq!(r.next(), Some(10));
assert_eq!(r.next(), Some(11));
assert_eq!(r.next(), Some(12));
assert_eq!(r, 1..=0);
assert!(r.is_empty());
assert_eq!(r.next(), None);

let mut r = 10..=12;
assert_eq!(r.next_back(), Some(12));
assert_eq!(r.next_back(), Some(11));
assert_eq!(r.next_back(), Some(10));
assert_eq!(r, 1..=0);
assert!(r.is_empty());
assert_eq!(r.next_back(), None);

let mut r = 10..=12;
assert_eq!(r.nth(2), Some(12));
assert_eq!(r, 1..=0);
assert!(r.is_empty());
assert_eq!(r.next(), None);

let mut r = 10..=12;
assert_eq!(r.nth(5), None);
assert_eq!(r, 1..=0);
assert!(r.is_empty());
assert_eq!(r.next(), None);

let mut r = 100..=10;
assert_eq!(r.next(), None);
assert!(r.is_empty());
assert_eq!(r.next(), None);
assert_eq!(r.next(), None);
assert_eq!(r, 100..=10);

let mut r = 100..=10;
assert_eq!(r.next_back(), None);
assert!(r.is_empty());
assert_eq!(r.next_back(), None);
assert_eq!(r.next_back(), None);
assert_eq!(r, 100..=10);
}

Expand Down Expand Up @@ -1428,9 +1470,10 @@ fn test_range_inclusive_nth() {
assert_eq!(r.nth(2), Some(15));
assert_eq!(r, 16..=20);
assert_eq!(r.is_empty(), false);
assert_eq!(ExactSizeIterator::is_empty(&r), false);
assert_eq!(r.nth(10), None);
assert_eq!(r.is_empty(), true);
assert_eq!(r, 1..=0); // We may not want to document/promise this detail
assert_eq!(ExactSizeIterator::is_empty(&r), true);
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to be a concern for end users?

Copy link
Member

Choose a reason for hiding this comment

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

ExactSizeIterator::is_empty (also unstable)

ah, nvm

Copy link
Member Author

Choose a reason for hiding this comment

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

Range::is_empty is strictly more general, so it's not a problem in a world where everything is stable. It may be a slight hiccup in nightly, but I would expect it to be solved in the wild by adding the new feature gate, rather than the annoying UFCS. Here I just changed it to the other method because I didn't want to change what the test was testing. (It would be a problem if we wanted to stabilize ExactSizeIterator::is_empty first, as mentioned in the OP, but I feel like this should be less controversial than the new trait method, especially since there are a bunch of iterators that aren't ExactSize that could provide an efficient is_empty.)

Copy link
Member Author

@scottmcm scottmcm Feb 10, 2018

Choose a reason for hiding this comment

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

Changed RangeInclusive exhaustion tests to check is_empty, updated the docs to unspecify the post-iteration value of a RangeInclusive, and added some more Range tests using its is_empty.

}

#[test]
Expand Down Expand Up @@ -1514,11 +1557,11 @@ fn test_range_inclusive_folds() {

let mut it = 10..=20;
assert_eq!(it.try_fold(0, |a,b| Some(a+b)), Some(165));
assert_eq!(it, 1..=0);
assert!(it.is_empty());

let mut it = 10..=20;
assert_eq!(it.try_rfold(0, |a,b| Some(a+b)), Some(165));
assert_eq!(it, 1..=0);
assert!(it.is_empty());
}

#[test]
Expand Down
1 change: 1 addition & 0 deletions src/libcore/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#![feature(iter_rfold)]
#![feature(nonzero)]
#![feature(pattern)]
#![feature(range_is_empty)]
#![feature(raw)]
#![feature(refcell_replace_swap)]
#![feature(sip_hash_13)]
Expand Down
24 changes: 24 additions & 0 deletions src/libcore/tests/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,27 @@ fn test_range_inclusive() {
assert_eq!(r.size_hint(), (0, Some(0)));
assert_eq!(r.next(), None);
}


#[test]
fn test_range_is_empty() {
use core::f32::*;

assert!(!(0.0 .. 10.0).is_empty());
assert!( (-0.0 .. 0.0).is_empty());
assert!( (10.0 .. 0.0).is_empty());

assert!(!(NEG_INFINITY .. INFINITY).is_empty());
assert!( (EPSILON .. NAN).is_empty());
assert!( (NAN .. EPSILON).is_empty());
assert!( (NAN .. NAN).is_empty());

assert!(!(0.0 ..= 10.0).is_empty());
assert!(!(-0.0 ..= 0.0).is_empty());
assert!( (10.0 ..= 0.0).is_empty());

assert!(!(NEG_INFINITY ..= INFINITY).is_empty());
assert!( (EPSILON ..= NAN).is_empty());
assert!( (NAN ..= EPSILON).is_empty());
assert!( (NAN ..= NAN).is_empty());
}
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about a test that calls next on a Range{Inclusive} and makes sure it transitions from not empty to empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thought; I'll change all of these to look for is_empty instead of 1..=0: https://github.com/rust-lang/rust/blob/master/src/libcore/tests/iter.rs#L1433