Add Range[Inclusive]::is_empty #48087

scottmcm · 2018-02-09T09:58:01Z

During rust-lang/rfcs#1980, it was discussed that figuring out whether a range is empty was subtle, and thus there should be a clear and obvious way to do it. It can't just be ExactSizeIterator::is_empty (also unstable) because not all ranges are ExactSize -- such as Range<i64> and RangeInclusive<usize>.

Things to ponder:

Unless this is stabilized first, this makes stabilizing ExactSizeIterator::is_empty more icky, since this hides that.
This is only on Range and RangeInclusive, as those are the only ones where it's interesting. But one could argue that it should be on more for consistency, or on RangeArgument instead.
The bound on this is PartialOrd, since that works ok (see tests for float examples) and is consistent with contains. But ranges like NAN..=NANare kinda weird.
~~There's not a real issue number on this yet~~

During the RFC, it was discussed that figuring out whether a range is empty was subtle, and thus there should be a clear and obvious way to do it. It can't just be ExactSizeIterator::is_empty (also unstable) because not all ranges are ExactSize -- not even Range<i32> or RangeInclusive<usize>.

rust-highfive · 2018-02-09T09:58:04Z

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

shepmaster · 2018-02-09T23:46:10Z

src/libcore/iter/traits.rs

@@ -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);


Why is this related to this PR?

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.

shepmaster · 2018-02-09T23:48:44Z

src/libcore/tests/iter.rs

    assert_eq!(r.nth(10), None);
-    assert_eq!(r.is_empty(), true);
+    assert_eq!(ExactSizeIterator::is_empty(&r), true);


Is this going to be a concern for end users?

ExactSizeIterator::is_empty (also unstable)

ah, nvm

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.)

shepmaster · 2018-02-09T23:49:46Z

The code seems reasonable to me, but since @kennytm has been thinking about ranges lately...

r? @kennytm

shepmaster · 2018-02-09T23:51:15Z

src/libcore/tests/ops.rs

+    assert!( (EPSILON ..= NAN).is_empty());
+    assert!( (NAN ..= EPSILON).is_empty());
+    assert!( (NAN ..= NAN).is_empty());
+}


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

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

scottmcm · 2018-02-10T01:58:27Z

src/libcore/tests/iter.rs

    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);


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.

kennytm · 2018-02-10T14:53:18Z

Since you're defining is_empty() to floating point types as well, perhaps spell out in the documentation that a..b is not empty when a < b (similar for a..=b).

LGTM other than this documentation bit, but I'd also like someone from @rust-lang/libs to sign-off.

alexcrichton · 2018-02-10T17:12:32Z

Looks good to me! Especially as unstable I think this can land when it's good to go

…h NAN

scottmcm · 2018-02-11T00:35:22Z

Added the start < end and start <= end to the doc comments next to the description of what the range contains, and added a doctest for each showing the behaviour with NAN.

kennytm · 2018-02-11T06:37:47Z

@bors r=kennytm,alexcrichton

bors · 2018-02-11T06:37:48Z

📌 Commit 22b0489 has been approved by kennytm,alexcrichton

…alexcrichton Add Range[Inclusive]::is_empty During rust-lang/rfcs#1980, it was discussed that figuring out whether a range is empty was subtle, and thus there should be a clear and obvious way to do it. It can't just be ExactSizeIterator::is_empty (also unstable) because not all ranges are ExactSize -- such as `Range<i64>` and `RangeInclusive<usize>`. Things to ponder: - Unless this is stabilized first, this makes stabilizing ExactSizeIterator::is_empty more icky, since this hides that. - This is only on `Range` and `RangeInclusive`, as those are the only ones where it's interesting. But one could argue that it should be on more for consistency, or on RangeArgument instead. - The bound on this is PartialOrd, since that works ok (see tests for float examples) and is consistent with `contains`. But ranges like `NAN..=NAN`_are_ kinda weird. - [x] ~~There's not a real issue number on this yet~~

Rollup of 8 pull requests - Successful merges: #47846, #48033, #48087, #48114, #48126, #48130, #48133, #48151 - Failed merges:

…alexcrichton Add Range[Inclusive]::is_empty During rust-lang/rfcs#1980, it was discussed that figuring out whether a range is empty was subtle, and thus there should be a clear and obvious way to do it. It can't just be ExactSizeIterator::is_empty (also unstable) because not all ranges are ExactSize -- such as `Range<i64>` and `RangeInclusive<usize>`. Things to ponder: - Unless this is stabilized first, this makes stabilizing ExactSizeIterator::is_empty more icky, since this hides that. - This is only on `Range` and `RangeInclusive`, as those are the only ones where it's interesting. But one could argue that it should be on more for consistency, or on RangeArgument instead. - The bound on this is PartialOrd, since that works ok (see tests for float examples) and is consistent with `contains`. But ranges like `NAN..=NAN`_are_ kinda weird. - [x] ~~There's not a real issue number on this yet~~

Rollup of 14 pull requests - Successful merges: #47784, #47846, #48033, #48083, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48163, #48165, #48167 - Failed merges:

…alexcrichton Add Range[Inclusive]::is_empty During rust-lang/rfcs#1980, it was discussed that figuring out whether a range is empty was subtle, and thus there should be a clear and obvious way to do it. It can't just be ExactSizeIterator::is_empty (also unstable) because not all ranges are ExactSize -- such as `Range<i64>` and `RangeInclusive<usize>`. Things to ponder: - Unless this is stabilized first, this makes stabilizing ExactSizeIterator::is_empty more icky, since this hides that. - This is only on `Range` and `RangeInclusive`, as those are the only ones where it's interesting. But one could argue that it should be on more for consistency, or on RangeArgument instead. - The bound on this is PartialOrd, since that works ok (see tests for float examples) and is consistent with `contains`. But ranges like `NAN..=NAN`_are_ kinda weird. - [x] ~~There's not a real issue number on this yet~~

Rollup of 22 pull requests - Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035 - Failed merges:

Rollup of 23 pull requests - Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035, #48210 - Failed merges:

rust-highfive assigned shepmaster Feb 9, 2018

Fix tidy

7fe182f

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2018

shepmaster reviewed Feb 9, 2018

View reviewed changes

rust-highfive assigned kennytm and unassigned shepmaster Feb 9, 2018

shepmaster reviewed Feb 9, 2018

View reviewed changes

Use is_empty in range iteration exhaustion tests

b5cb393

scottmcm commented Feb 10, 2018

View reviewed changes

scottmcm mentioned this pull request Feb 10, 2018

Tracking issue for Range[Inclusive]::is_empty (feature range_is_empty) #48111

Closed

range_is_empty tracking issue is rust-lang#48111

6f70a11

kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 10, 2018

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2018

Add the emptiness condition to the docs; add a PartialOrd example wit…

22b0489

…h NAN

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2018

kennytm mentioned this pull request Feb 12, 2018

Rollup of 8 pull requests #48158

Closed

bors added a commit that referenced this pull request Feb 12, 2018

Auto merge of #48158 - kennytm:rollup, r=kennytm

710e972

Rollup of 8 pull requests - Successful merges: #47846, #48033, #48087, #48114, #48126, #48130, #48133, #48151 - Failed merges:

kennytm mentioned this pull request Feb 13, 2018

Rollup of 14 pull requests #48177

Closed

kennytm mentioned this pull request Feb 14, 2018

Rollup of 23 pull requests #48203

Merged

kennytm merged commit 22b0489 into rust-lang:master Feb 15, 2018

scottmcm deleted the range_is_empty branch February 20, 2018 01:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Range[Inclusive]::is_empty #48087

Add Range[Inclusive]::is_empty #48087

scottmcm commented Feb 9, 2018 •

edited

Loading

rust-highfive commented Feb 9, 2018

shepmaster Feb 9, 2018

scottmcm Feb 9, 2018

shepmaster Feb 9, 2018

shepmaster Feb 9, 2018

scottmcm Feb 9, 2018

shepmaster commented Feb 9, 2018

shepmaster Feb 9, 2018

scottmcm Feb 9, 2018

scottmcm Feb 10, 2018 •

edited

Loading

kennytm commented Feb 10, 2018 •

edited

Loading

alexcrichton commented Feb 10, 2018

scottmcm commented Feb 11, 2018

kennytm commented Feb 11, 2018

bors commented Feb 11, 2018

Add Range[Inclusive]::is_empty #48087

Add Range[Inclusive]::is_empty #48087

Conversation

scottmcm commented Feb 9, 2018 • edited Loading

rust-highfive commented Feb 9, 2018

shepmaster Feb 9, 2018

Choose a reason for hiding this comment

scottmcm Feb 9, 2018

Choose a reason for hiding this comment

shepmaster Feb 9, 2018

Choose a reason for hiding this comment

shepmaster Feb 9, 2018

Choose a reason for hiding this comment

scottmcm Feb 9, 2018

Choose a reason for hiding this comment

shepmaster commented Feb 9, 2018

shepmaster Feb 9, 2018

Choose a reason for hiding this comment

scottmcm Feb 9, 2018

Choose a reason for hiding this comment

scottmcm Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

kennytm commented Feb 10, 2018 • edited Loading

alexcrichton commented Feb 10, 2018

scottmcm commented Feb 11, 2018

kennytm commented Feb 11, 2018

bors commented Feb 11, 2018

scottmcm commented Feb 9, 2018 •

edited

Loading

scottmcm Feb 10, 2018 •

edited

Loading

kennytm commented Feb 10, 2018 •

edited

Loading