Skip to content

Move Range*::contains to a single default impl on RangeBounds #49130

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

Merged
merged 4 commits into from
Apr 16, 2018
Merged
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
157 changes: 123 additions & 34 deletions src/libcore/ops/range.rs
Original file line number Diff line number Diff line change
@@ -100,17 +100,28 @@ impl<Idx: PartialOrd<Idx>> Range<Idx> {
/// ```
/// #![feature(range_contains)]
///
/// assert!(!(3..5).contains(2));
/// assert!( (3..5).contains(3));
/// assert!( (3..5).contains(4));
/// assert!(!(3..5).contains(5));
/// use std::f32;
///
/// assert!(!(3..3).contains(3));
/// assert!(!(3..2).contains(3));
/// assert!(!(3..5).contains(&2));
/// assert!( (3..5).contains(&3));
/// assert!( (3..5).contains(&4));
/// assert!(!(3..5).contains(&5));
///
/// assert!(!(3..3).contains(&3));
/// assert!(!(3..2).contains(&3));
///
/// assert!( (0.0..1.0).contains(&0.5));
/// assert!(!(0.0..1.0).contains(&f32::NAN));
/// assert!(!(0.0..f32::NAN).contains(&0.5));
/// assert!(!(f32::NAN..1.0).contains(&0.5));
/// ```
#[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)
pub fn contains<U>(&self, item: &U) -> bool
where
Idx: PartialOrd<U>,
U: ?Sized + PartialOrd<Idx>,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}

/// Returns `true` if the range contains no items.
@@ -179,7 +190,6 @@ impl<Idx: fmt::Debug> fmt::Debug for RangeFrom<Idx> {
}
}

#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
impl<Idx: PartialOrd<Idx>> RangeFrom<Idx> {
/// Returns `true` if `item` is contained in the range.
///
@@ -188,12 +198,23 @@ impl<Idx: PartialOrd<Idx>> RangeFrom<Idx> {
/// ```
/// #![feature(range_contains)]
///
/// assert!(!(3..).contains(2));
/// assert!( (3..).contains(3));
/// assert!( (3..).contains(1_000_000_000));
/// use std::f32;
///
/// assert!(!(3..).contains(&2));
/// assert!( (3..).contains(&3));
/// assert!( (3..).contains(&1_000_000_000));
///
/// assert!( (0.0..).contains(&0.5));
/// assert!(!(0.0..).contains(&f32::NAN));
/// assert!(!(f32::NAN..).contains(&0.5));
/// ```
pub fn contains(&self, item: Idx) -> bool {
(self.start <= item)
#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
pub fn contains<U>(&self, item: &U) -> bool
where
Idx: PartialOrd<U>,
U: ?Sized + PartialOrd<Idx>,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}
}

@@ -250,7 +271,6 @@ impl<Idx: fmt::Debug> fmt::Debug for RangeTo<Idx> {
}
}

#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
impl<Idx: PartialOrd<Idx>> RangeTo<Idx> {
/// Returns `true` if `item` is contained in the range.
///
@@ -259,12 +279,23 @@ impl<Idx: PartialOrd<Idx>> RangeTo<Idx> {
/// ```
/// #![feature(range_contains)]
///
/// assert!( (..5).contains(-1_000_000_000));
/// assert!( (..5).contains(4));
/// assert!(!(..5).contains(5));
/// use std::f32;
///
/// assert!( (..5).contains(&-1_000_000_000));
/// assert!( (..5).contains(&4));
/// assert!(!(..5).contains(&5));
///
/// assert!( (..1.0).contains(&0.5));
/// assert!(!(..1.0).contains(&f32::NAN));
/// assert!(!(..f32::NAN).contains(&0.5));
/// ```
pub fn contains(&self, item: Idx) -> bool {
(item < self.end)
#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
pub fn contains<U>(&self, item: &U) -> bool
where
Idx: PartialOrd<U>,
U: ?Sized + PartialOrd<Idx>,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}
}

@@ -318,18 +349,29 @@ impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
/// ```
/// #![feature(range_contains)]
///
/// assert!(!(3..=5).contains(2));
/// assert!( (3..=5).contains(3));
/// assert!( (3..=5).contains(4));
/// assert!( (3..=5).contains(5));
/// assert!(!(3..=5).contains(6));
/// use std::f32;
///
/// assert!(!(3..=5).contains(&2));
/// assert!( (3..=5).contains(&3));
/// assert!( (3..=5).contains(&4));
/// assert!( (3..=5).contains(&5));
/// assert!(!(3..=5).contains(&6));
///
/// assert!( (3..=3).contains(3));
/// assert!(!(3..=2).contains(3));
/// assert!( (3..=3).contains(&3));
/// assert!(!(3..=2).contains(&3));
///
/// assert!( (0.0..=1.0).contains(&1.0));
/// assert!(!(0.0..=1.0).contains(&f32::NAN));
/// assert!(!(0.0..=f32::NAN).contains(&0.0));
/// assert!(!(f32::NAN..=1.0).contains(&1.0));
/// ```
#[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
pub fn contains<U>(&self, item: &U) -> bool
where
Idx: PartialOrd<U>,
U: ?Sized + PartialOrd<Idx>,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}

/// Returns `true` if the range contains no items.
@@ -431,12 +473,23 @@ impl<Idx: PartialOrd<Idx>> RangeToInclusive<Idx> {
/// ```
/// #![feature(range_contains)]
///
/// assert!( (..=5).contains(-1_000_000_000));
/// assert!( (..=5).contains(5));
/// assert!(!(..=5).contains(6));
/// use std::f32;
///
/// assert!( (..=5).contains(&-1_000_000_000));
/// assert!( (..=5).contains(&5));
/// assert!(!(..=5).contains(&6));
///
/// assert!( (..=1.0).contains(&1.0));
/// assert!(!(..=1.0).contains(&f32::NAN));
/// assert!(!(..=f32::NAN).contains(&0.5));
/// ```
pub fn contains(&self, item: Idx) -> bool {
(item <= self.end)
#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
pub fn contains<U>(&self, item: &U) -> bool
where
Idx: PartialOrd<U>,
U: ?Sized + PartialOrd<Idx>,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}
}

@@ -537,6 +590,42 @@ pub trait RangeBounds<T: ?Sized> {
/// # }
/// ```
fn end(&self) -> Bound<&T>;


/// Returns `true` if `item` is contained in the range.
///
/// # Examples
///
/// ```
/// #![feature(range_contains)]
///
/// use std::f32;
///
/// assert!( (3..5).contains(&4));
/// assert!(!(3..5).contains(&2));
///
/// assert!( (0.0..1.0).contains(&0.5));
/// assert!(!(0.0..1.0).contains(&f32::NAN));
/// assert!(!(0.0..f32::NAN).contains(&0.5));
/// assert!(!(f32::NAN..1.0).contains(&0.5));
#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
fn contains<U>(&self, item: &U) -> bool
where
T: PartialOrd<U>,
Copy link
Member

Choose a reason for hiding this comment

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

Re-mentioning the T: PartialOrd<U> vs U: PartialOrd<T> vs both discussion, since I'm not sure it was fully resolved.

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 think we agreed on not U: PartialOrd<T>, but the other two are still options.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still in favour of where T: PartialOrd<U>, U: PartialOrd<T> for compatibility. For anyone implementing these traits correctly, there'll be no inconvenience, but it makes it easier in the future to modify in a way that's backwards-compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's definitely the advantage there of making sure that everything is implemented correctly, however it does raise the question of consistency. Do we think this is a small enough issue that it's more worthwhile to be consistent with the rest of std? Are there plans to address this issue in a more broad way that would apply everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

The rest of std isn't consistent. For example, Vec::contains isn't even generic over the search type. The issue is that, in the past, the methods weren't defined generally enough, and when people come back to try to fix that, there are type inference issues.

The general practice now is "make new methods behave correctly" and we'll try to fix the old ones when it's possible (e.g. with #27336 and #46946). These aren't going to be done very soon, though, so as someone who has been bitten by bad signatures in the past, a little more inconsistency is far preferable.

Copy link
Contributor Author

@smmalis37 smmalis37 Apr 11, 2018

Choose a reason for hiding this comment

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

Sounds good to me, I'll make the change once I'm home from work.

U: ?Sized + PartialOrd<T>,
{
(match self.start() {
Included(ref start) => *start <= item,
Excluded(ref start) => *start < item,
Unbounded => true,
})
&&
(match self.end() {
Included(ref end) => item <= *end,
Excluded(ref end) => item < *end,
Unbounded => true,
})
}
}

use self::Bound::{Excluded, Included, Unbounded};
4 changes: 2 additions & 2 deletions src/librustc_errors/emitter.rs
Original file line number Diff line number Diff line change
@@ -1389,8 +1389,8 @@ fn num_overlap(a_start: usize, a_end: usize, b_start: usize, b_end:usize, inclus
} else {
0
};
(b_start..b_end + extra).contains(a_start) ||
(a_start..a_end + extra).contains(b_start)
(b_start..b_end + extra).contains(&a_start) ||
(a_start..a_end + extra).contains(&b_start)
}
fn overlaps(a1: &Annotation, a2: &Annotation, padding: usize) -> bool {
num_overlap(a1.start_col, a1.end_col + padding, a2.start_col, a2.end_col, false)
8 changes: 4 additions & 4 deletions src/librustc_mir/borrow_check/nll/universal_regions.rs
Original file line number Diff line number Diff line change
@@ -259,18 +259,18 @@ impl<'tcx> UniversalRegions<'tcx> {

/// True if `r` is a member of this set of universal regions.
pub fn is_universal_region(&self, r: RegionVid) -> bool {
(FIRST_GLOBAL_INDEX..self.num_universals).contains(r.index())
(FIRST_GLOBAL_INDEX..self.num_universals).contains(&r.index())
}

/// Classifies `r` as a universal region, returning `None` if this
/// is not a member of this set of universal regions.
pub fn region_classification(&self, r: RegionVid) -> Option<RegionClassification> {
let index = r.index();
if (FIRST_GLOBAL_INDEX..self.first_extern_index).contains(index) {
if (FIRST_GLOBAL_INDEX..self.first_extern_index).contains(&index) {
Some(RegionClassification::Global)
} else if (self.first_extern_index..self.first_local_index).contains(index) {
} else if (self.first_extern_index..self.first_local_index).contains(&index) {
Some(RegionClassification::External)
} else if (self.first_local_index..self.num_universals).contains(index) {
} else if (self.first_local_index..self.num_universals).contains(&index) {
Some(RegionClassification::Local)
} else {
None