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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Apr 16, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f5a367c
Update based on RangeBounds trait being moved to libcore.
smmalis37 Apr 7, 2018
16f30c2
fix tests
smmalis37 Apr 8, 2018
249dc9e
Add float NaN tests.
smmalis37 Apr 8, 2018
51f24ec
Add symmetric requirement of PartialOrd.
smmalis37 Apr 11, 2018
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}

/// Returns `true` if the range contains no items.
Expand Down Expand Up @@ -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.
///
Expand All @@ -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,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}
}

Expand Down Expand Up @@ -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.
///
Expand All @@ -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,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}
}

Expand Down Expand Up @@ -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,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}

/// Returns `true` if the range contains no items.
Expand Down Expand Up @@ -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,
{
<Self as RangeBounds<Idx>>::contains(self, item)
}
}

Expand Down Expand Up @@ -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 •
edited
Loading

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,
{
(match self.start() {
Included(ref start) => *start <= item,
Excluded(ref start) => *start < item,
Unbounded => true,
})
&&
(match self.end() {
Included(ref end) => *end >= item,
Excluded(ref end) => *end > item,
Unbounded => true,
})
}
}

use self::Bound::{Excluded, Included, Unbounded};
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_errors/emitter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_mir/borrow_check/nll/universal_regions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down