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

Implement RangeArgument for RangeInclusive and RangeInclusiveTo #32681

Closed
wants to merge 3 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
68 changes: 52 additions & 16 deletions src/libcollections/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,47 +15,83 @@
//! Range syntax.

use core::option::Option::{self, None, Some};
use core::ops::{RangeFull, Range, RangeTo, RangeFrom};
use core::ops::{RangeFull, Range, RangeTo, RangeFrom, RangeInclusive, RangeToInclusive};

/// **RangeArgument** is implemented by Rust's built-in range types, produced
/// by range syntax like `..`, `a..`, `..b` or `c..d`.
pub trait RangeArgument<T> {
/// Start index (inclusive)
///
/// Return start value if present, else `None`.
fn start(&self) -> Option<&T> {
fn start(&self) -> Option<T> {
None
}

/// End index (exclusive)
///
/// Return end value if present, else `None`.
fn end(&self) -> Option<&T> {
fn end(&self) -> Option<T> {
None
}
}

// FIXME add inclusive ranges to RangeArgument

impl<T> RangeArgument<T> for RangeFull {}

impl<T> RangeArgument<T> for RangeFrom<T> {
fn start(&self) -> Option<&T> {
Some(&self.start)
impl<T: Copy> RangeArgument<T> for RangeFrom<T> {
fn start(&self) -> Option<T> {
Some(self.start)
}
}

impl<T> RangeArgument<T> for RangeTo<T> {
fn end(&self) -> Option<&T> {
Some(&self.end)
impl<T: Copy> RangeArgument<T> for RangeTo<T> {
fn end(&self) -> Option<T> {
Some(self.end)
}
}

impl<T> RangeArgument<T> for Range<T> {
fn start(&self) -> Option<&T> {
Some(&self.start)
impl<T: Copy> RangeArgument<T> for Range<T> {
fn start(&self) -> Option<T> {
Some(self.start)
}
fn end(&self) -> Option<T> {
Some(self.end)
}
fn end(&self) -> Option<&T> {
Some(&self.end)
}

macro_rules! inclusive {
($Int: ty) => {
impl RangeArgument<$Int> for RangeToInclusive<$Int> {
fn end(&self) -> Option<$Int> {
Some(self.end.checked_add(1).expect("inclusive range to maximum usize"))
}
}

impl RangeArgument<$Int> for RangeInclusive<$Int> {
fn start(&self) -> Option<$Int> {
match *self {
RangeInclusive::Empty { at } => Some(at),
RangeInclusive::NonEmpty { start, .. } => Some(start),
}
}
fn end(&self) -> Option<$Int> {
match *self {
RangeInclusive::Empty { at } => Some(at),
RangeInclusive::NonEmpty { end, .. } => {
Some(end.checked_add(1).expect("inclusive range to maximum usize"))
Copy link
Contributor

Choose a reason for hiding this comment

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

The message is wrong, isn't it? When this is expanded for u8, (0...255).end() will panic here. Same for the one above.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the best solution without changing the semantics of RangeArgument, but I'm not sure that's the best approach. It feels gross that you can't really use RangeArgument for a range that goes to the rails (like 0...255), which was the main motivation for implementing inclusive ranges. IMO it would be better to change RangeArgument to return an enum like enum Bounded<T> { Inclusive(&T), Exclusive(&T) } (maybe clones instead of references, haven't thought it through) at both ends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RangeArgument is only used in std with ranges of usize, so this would be 0...4294967295 or 0...18446744073709551615 rather than 0..255. We could change RangeArgument::end (I don’t think changing start is useful) to return an enum as you suggest, but that would only push the panic to each caller (namely Vec::drain, String::drain, and VecDeque::drain) since in-memory collections can not have an item at index usize::MAX (this would make the length >= usize::MAX + 1).

(For what it’s worth, my opinion is that inclusive ranges are not useful for slicing, only as iterators. I’m only making this PR to push for stabilizing RangeArgument.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm well it's easy enough to fix the message to concat!("inclusive range to maximum ", stringify!($Int)) at least.

I see your point but I'm not sure I like privileging usize. And I don't like adding more panics to the standard library. But there's no reason to listen to me! I also understand the desire for stabilization, of course, but it may be premature since I think that range syntax is going to become more flexible (@aturon keeps reminding me to write that RFC...).

}
}
}
}
}
}

inclusive!(u8);
inclusive!(u16);
inclusive!(u32);
inclusive!(u64);
inclusive!(usize);
inclusive!(i8);
inclusive!(i16);
inclusive!(i32);
inclusive!(i64);
inclusive!(isize);
4 changes: 2 additions & 2 deletions src/libcollections/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1269,8 +1269,8 @@ impl String {
// Because the range removal happens in Drop, if the Drain iterator is leaked,
// the removal will not happen.
let len = self.len();
let start = *range.start().unwrap_or(&0);
let end = *range.end().unwrap_or(&len);
let start = range.start().unwrap_or(0);
let end = range.end().unwrap_or(len);

// Take out two simultaneous borrows. The &mut String won't be accessed
// until iteration is over, in Drop.
Expand Down
4 changes: 2 additions & 2 deletions src/libcollections/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -805,8 +805,8 @@ impl<T> Vec<T> {
// the hole, and the vector length is restored to the new length.
//
let len = self.len();
let start = *range.start().unwrap_or(&0);
let end = *range.end().unwrap_or(&len);
let start = range.start().unwrap_or(0);
let end = range.end().unwrap_or(len);
assert!(start <= end);
assert!(end <= len);

Expand Down
4 changes: 2 additions & 2 deletions src/libcollections/vec_deque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -813,8 +813,8 @@ impl<T> VecDeque<T> {
// and the head/tail values will be restored correctly.
//
let len = self.len();
let start = *range.start().unwrap_or(&0);
let end = *range.end().unwrap_or(&len);
let start = range.start().unwrap_or(0);
let end = range.end().unwrap_or(len);
assert!(start <= end, "drain lower bound was too large");
assert!(end <= len, "drain upper bound was too large");

Expand Down
1 change: 1 addition & 0 deletions src/libcollectionstest/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#![feature(const_fn)]
#![feature(fn_traits)]
#![feature(enumset)]
#![feature(inclusive_range_syntax)]
#![feature(iter_arith)]
#![feature(map_entry_keys)]
#![feature(pattern)]
Expand Down
19 changes: 19 additions & 0 deletions src/libcollectionstest/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,25 @@ fn test_drain_range() {
assert_eq!(v, &[(), ()]);
}

#[test]
fn test_drain_range_inclusive() {
let mut vec = vec![1, 2, 3, 4, 5];
let mut vec2 = vec![];
for i in vec.drain(1...3) {
vec2.push(i);
}
assert_eq!(vec, [1, 5]);
assert_eq!(vec2, [2, 3, 4]);

let mut vec = vec![1, 2, 3, 4, 5];
let mut vec2 = vec![];
for i in vec.drain(...3) {
vec2.push(i);
}
assert_eq!(vec, [5]);
assert_eq!(vec2, [1, 2, 3, 4]);
}

#[test]
fn test_into_boxed_slice() {
let xs = vec![1, 2, 3];
Expand Down
2 changes: 0 additions & 2 deletions src/libcore/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,8 +534,6 @@ fn slice_index_order_fail(index: usize, end: usize) -> ! {
panic!("slice index starts at {} but ends at {}", index, end);
}

// FIXME implement indexing with inclusive ranges

/// Implements slicing with syntax `&self[begin .. end]`.
///
/// Returns a slice of self for the index range [`begin`..`end`).
Expand Down