Auto merge of #68835 - CAD97:sound-range-inclusive, r=Mark-Simulacrum
Remove problematic specialization from RangeInclusive Fixes #67194 using the approach [outlined by Mark-Simulacrum](https://github.com/rust-lang/rust/issues/67194#issuecomment-581669549). > I believe the property we want is that if `PartialEq(&self, &other) == true`, then `self.next() == other.next()`. It is true that this is satisfied by removing the specialization and always doing `is_empty.unwrap_or_default()`; the "wrong" behavior there arises from calling `next()` having an effect on initially empty ranges, as we should be in `is_empty = true` but are not (yet) there. It might be possible to detect that the current state is always empty (i.e., `start > end`) and then not fill in the empty slot. I think this might solve the problem without regressing tests; however, this could have performance implications. > That approach essentially states that we only use the `is_empty` slot for cases where `start <= end`. That means that `Idx: !Step` and `start > end` would both behave the same, and correctly -- we do not need the boolean if we're not ever going to emit any values from the iterator. This is implemented here by replacing the `is_empty: Option<bool>` slot with an `exhausted: bool` slot. This flag is - `false` upon construction, - `false` when iteration has not yielded an element -- importantly, this means it is always `false` for an iterator empty by construction, - `false` when iteration has yielded an element and the iterator is not exhausted, and - only `true` when iteration has been used to exhaust the iterator. For completeness, this also adds a note to the `Debug` representation to note when the range is exhausted.
This commit is contained in:
commit
e6ec0d125e
@ -341,16 +341,15 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
|
||||
|
||||
#[inline]
|
||||
fn next(&mut self) -> Option<A> {
|
||||
self.compute_is_empty();
|
||||
if self.is_empty.unwrap_or_default() {
|
||||
if self.is_empty() {
|
||||
return None;
|
||||
}
|
||||
let is_iterating = self.start < self.end;
|
||||
self.is_empty = Some(!is_iterating);
|
||||
Some(if is_iterating {
|
||||
let n = self.start.add_one();
|
||||
mem::replace(&mut self.start, n)
|
||||
} else {
|
||||
self.exhausted = true;
|
||||
self.start.clone()
|
||||
})
|
||||
}
|
||||
@ -369,8 +368,7 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
|
||||
|
||||
#[inline]
|
||||
fn nth(&mut self, n: usize) -> Option<A> {
|
||||
self.compute_is_empty();
|
||||
if self.is_empty.unwrap_or_default() {
|
||||
if self.is_empty() {
|
||||
return None;
|
||||
}
|
||||
|
||||
@ -379,13 +377,12 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
|
||||
|
||||
match plus_n.partial_cmp(&self.end) {
|
||||
Some(Less) => {
|
||||
self.is_empty = Some(false);
|
||||
self.start = plus_n.add_one();
|
||||
return Some(plus_n);
|
||||
}
|
||||
Some(Equal) => {
|
||||
self.is_empty = Some(true);
|
||||
self.start = plus_n.clone();
|
||||
self.exhausted = true;
|
||||
return Some(plus_n);
|
||||
}
|
||||
_ => {}
|
||||
@ -393,7 +390,7 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
|
||||
}
|
||||
|
||||
self.start = self.end.clone();
|
||||
self.is_empty = Some(true);
|
||||
self.exhausted = true;
|
||||
None
|
||||
}
|
||||
|
||||
@ -404,8 +401,6 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
|
||||
F: FnMut(B, Self::Item) -> R,
|
||||
R: Try<Ok = B>,
|
||||
{
|
||||
self.compute_is_empty();
|
||||
|
||||
if self.is_empty() {
|
||||
return Try::from_ok(init);
|
||||
}
|
||||
@ -418,7 +413,7 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
|
||||
accum = f(accum, n)?;
|
||||
}
|
||||
|
||||
self.is_empty = Some(true);
|
||||
self.exhausted = true;
|
||||
|
||||
if self.start == self.end {
|
||||
accum = f(accum, self.start.clone())?;
|
||||
@ -447,24 +442,22 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
|
||||
impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
|
||||
#[inline]
|
||||
fn next_back(&mut self) -> Option<A> {
|
||||
self.compute_is_empty();
|
||||
if self.is_empty.unwrap_or_default() {
|
||||
if self.is_empty() {
|
||||
return None;
|
||||
}
|
||||
let is_iterating = self.start < self.end;
|
||||
self.is_empty = Some(!is_iterating);
|
||||
Some(if is_iterating {
|
||||
let n = self.end.sub_one();
|
||||
mem::replace(&mut self.end, n)
|
||||
} else {
|
||||
self.exhausted = true;
|
||||
self.end.clone()
|
||||
})
|
||||
}
|
||||
|
||||
#[inline]
|
||||
fn nth_back(&mut self, n: usize) -> Option<A> {
|
||||
self.compute_is_empty();
|
||||
if self.is_empty.unwrap_or_default() {
|
||||
if self.is_empty() {
|
||||
return None;
|
||||
}
|
||||
|
||||
@ -473,13 +466,12 @@ impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
|
||||
|
||||
match minus_n.partial_cmp(&self.start) {
|
||||
Some(Greater) => {
|
||||
self.is_empty = Some(false);
|
||||
self.end = minus_n.sub_one();
|
||||
return Some(minus_n);
|
||||
}
|
||||
Some(Equal) => {
|
||||
self.is_empty = Some(true);
|
||||
self.end = minus_n.clone();
|
||||
self.exhausted = true;
|
||||
return Some(minus_n);
|
||||
}
|
||||
_ => {}
|
||||
@ -487,7 +479,7 @@ impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
|
||||
}
|
||||
|
||||
self.end = self.start.clone();
|
||||
self.is_empty = Some(true);
|
||||
self.exhausted = true;
|
||||
None
|
||||
}
|
||||
|
||||
@ -498,8 +490,6 @@ impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
|
||||
F: FnMut(B, Self::Item) -> R,
|
||||
R: Try<Ok = B>,
|
||||
{
|
||||
self.compute_is_empty();
|
||||
|
||||
if self.is_empty() {
|
||||
return Try::from_ok(init);
|
||||
}
|
||||
@ -512,7 +502,7 @@ impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
|
||||
accum = f(accum, n)?;
|
||||
}
|
||||
|
||||
self.is_empty = Some(true);
|
||||
self.exhausted = true;
|
||||
|
||||
if self.start == self.end {
|
||||
accum = f(accum, self.start.clone())?;
|
||||
|
@ -340,24 +340,21 @@ pub struct RangeInclusive<Idx> {
|
||||
// support that mode.
|
||||
pub(crate) start: Idx,
|
||||
pub(crate) end: Idx,
|
||||
pub(crate) is_empty: Option<bool>,
|
||||
|
||||
// This field is:
|
||||
// - `None` when next() or next_back() was never called
|
||||
// - `Some(false)` when `start < end`
|
||||
// - `Some(true)` when `end < start`
|
||||
// - `Some(false)` when `start == end` and the range hasn't yet completed iteration
|
||||
// - `Some(true)` when `start == end` and the range has completed iteration
|
||||
// The field cannot be a simple `bool` because the `..=` constructor can
|
||||
// accept non-PartialOrd types, also we want the constructor to be const.
|
||||
// - `false` upon construction
|
||||
// - `false` when iteration has yielded an element and the iterator is not exhausted
|
||||
// - `true` when iteration has been used to exhaust the iterator
|
||||
//
|
||||
// This is required to support PartialEq and Hash without a PartialOrd bound or specialization.
|
||||
pub(crate) exhausted: bool,
|
||||
}
|
||||
|
||||
#[stable(feature = "inclusive_range", since = "1.26.0")]
|
||||
impl<Idx: PartialEq> PartialEq for RangeInclusive<Idx> {
|
||||
#[inline]
|
||||
fn eq(&self, other: &Self) -> bool {
|
||||
self.start == other.start
|
||||
&& self.end == other.end
|
||||
&& self.is_exhausted() == other.is_exhausted()
|
||||
self.start == other.start && self.end == other.end && self.exhausted == other.exhausted
|
||||
}
|
||||
}
|
||||
|
||||
@ -369,8 +366,7 @@ impl<Idx: Hash> Hash for RangeInclusive<Idx> {
|
||||
fn hash<H: Hasher>(&self, state: &mut H) {
|
||||
self.start.hash(state);
|
||||
self.end.hash(state);
|
||||
// Ideally we would hash `is_exhausted` here as well, but there's no
|
||||
// way for us to call it.
|
||||
self.exhausted.hash(state);
|
||||
}
|
||||
}
|
||||
|
||||
@ -389,7 +385,7 @@ impl<Idx> RangeInclusive<Idx> {
|
||||
#[rustc_promotable]
|
||||
#[rustc_const_stable(feature = "const_range_new", since = "1.32.0")]
|
||||
pub const fn new(start: Idx, end: Idx) -> Self {
|
||||
Self { start, end, is_empty: None }
|
||||
Self { start, end, exhausted: false }
|
||||
}
|
||||
|
||||
/// Returns the lower bound of the range (inclusive).
|
||||
@ -465,15 +461,10 @@ impl<Idx: fmt::Debug> fmt::Debug for RangeInclusive<Idx> {
|
||||
self.start.fmt(fmt)?;
|
||||
write!(fmt, "..=")?;
|
||||
self.end.fmt(fmt)?;
|
||||
Ok(())
|
||||
if self.exhausted {
|
||||
write!(fmt, " (exhausted)")?;
|
||||
}
|
||||
}
|
||||
|
||||
impl<Idx: PartialEq<Idx>> RangeInclusive<Idx> {
|
||||
// Returns true if this is a range that started non-empty, and was iterated
|
||||
// to exhaustion.
|
||||
fn is_exhausted(&self) -> bool {
|
||||
Some(true) == self.is_empty && self.start == self.end
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
@ -544,15 +535,7 @@ impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
|
||||
#[unstable(feature = "range_is_empty", reason = "recently added", issue = "48111")]
|
||||
#[inline]
|
||||
pub fn is_empty(&self) -> bool {
|
||||
self.is_empty.unwrap_or_else(|| !(self.start <= self.end))
|
||||
}
|
||||
|
||||
// If this range's `is_empty` is field is unknown (`None`), update it to be a concrete value.
|
||||
#[inline]
|
||||
pub(crate) fn compute_is_empty(&mut self) {
|
||||
if self.is_empty.is_none() {
|
||||
self.is_empty = Some(!(self.start <= self.end));
|
||||
}
|
||||
self.exhausted || !(self.start <= self.end)
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -25,28 +25,30 @@ pub fn check_foo2() -> u64 {
|
||||
}
|
||||
|
||||
// Simplified example of #45222
|
||||
|
||||
fn triangle_inc(n: u64) -> u64 {
|
||||
let mut count = 0;
|
||||
for j in 0 ..= n {
|
||||
count += j;
|
||||
}
|
||||
count
|
||||
}
|
||||
|
||||
// CHECK-LABEL: @check_triangle_inc
|
||||
#[no_mangle]
|
||||
pub fn check_triangle_inc() -> u64 {
|
||||
// CHECK: ret i64 5000050000
|
||||
triangle_inc(100000)
|
||||
}
|
||||
//
|
||||
// Temporarily disabled in #68835 to fix a soundness hole.
|
||||
//
|
||||
// fn triangle_inc(n: u64) -> u64 {
|
||||
// let mut count = 0;
|
||||
// for j in 0 ..= n {
|
||||
// count += j;
|
||||
// }
|
||||
// count
|
||||
// }
|
||||
//
|
||||
// // COMMENTEDCHECK-LABEL: @check_triangle_inc
|
||||
// #[no_mangle]
|
||||
// pub fn check_triangle_inc() -> u64 {
|
||||
// // COMMENTEDCHECK: ret i64 5000050000
|
||||
// triangle_inc(100000)
|
||||
// }
|
||||
|
||||
// Demo in #48012
|
||||
|
||||
fn foo3r(n: u64) -> u64 {
|
||||
let mut count = 0;
|
||||
(0..n).for_each(|_| {
|
||||
(0 ..= n).rev().for_each(|j| {
|
||||
(0..=n).rev().for_each(|j| {
|
||||
count += j;
|
||||
})
|
||||
});
|
||||
|
Loading…
Reference in New Issue
Block a user