Change RangeInclusive to a three-field struct.

Fix #45222.
rust-lang · Jun 18, 2018 · 0d7e9933d3cac85bc1f11dc0fec67fcad77784ca · 0d7e993
1 parent 7db82cc
commit 0d7e9933d3cac85bc1f11dc0fec67fcad77784ca
Unified Split

Showing with 155 additions and 97 deletions.

+30 −70 src/libcore/iter/range.rs

+31 −7 src/libcore/ops/range.rs

+10 −10 src/libcore/slice/mod.rs

+10 −10 src/libcore/str/mod.rs

+74 −0 src/test/codegen/issue-45222.rs
diff --git a/src/libcore/iter/range.rs b/src/libcore/iter/range.rs
@@ -10,7 +10,7 @@
 use convert::TryFrom;
 use mem;
-  
-use ops::{self, Add, Sub, Try};
+  
+use ops::{self, Add, Sub};
 use usize;
 use super::{FusedIterator, TrustedLen};
@@ -330,23 +330,23 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
    #[inline]
    fn next(&mut self) -> Option<A> {
-  
-        if self.start <= self.end {
-  
-            if self.start < self.end {
-  
-                let n = self.start.add_one();
-  
-                Some(mem::replace(&mut self.start, n))
-  
-            } else {
-  
-                let last = self.start.replace_one();
-  
-                self.end.replace_zero();
-  
-                Some(last)
-  
-            }
+  
+        if self.is_empty() {
+  
+            self.is_iterating = Some(false);
+  
+            return None;
+  
+        }
+  
+        if self.start < self.end {
+  
+            let n = self.start.add_one();
+  
+            self.is_iterating = Some(true);
+  
+            Some(mem::replace(&mut self.start, n))
        } else {
-  
-            None
+  
+            self.is_iterating = Some(false);
+  
+            Some(self.start.clone())
        }
    }
    #[inline]
    fn size_hint(&self) -> (usize, Option<usize>) {
-  
-        if !(self.start <= self.end) {
+  
+        if self.is_empty() {
            return (0, Some(0));
        }
@@ -358,25 +358,29 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
    #[inline]
    fn nth(&mut self, n: usize) -> Option<A> {
+  
+        if self.is_empty() {
+  
+            self.is_iterating = Some(false);
+  
+            return None;
+  
+        }
+  
+
        if let Some(plus_n) = self.start.add_usize(n) {
            use cmp::Ordering::*;
            match plus_n.partial_cmp(&self.end) {
                Some(Less) => {
+  
+                    self.is_iterating = Some(true);
                    self.start = plus_n.add_one();
                    return Some(plus_n)
                }
                Some(Equal) => {
-  
-                    self.start.replace_one();
-  
-                    self.end.replace_zero();
+  
+                    self.is_iterating = Some(false);
                    return Some(plus_n)
                }
                _ => {}
            }
        }
-  
-        self.start.replace_one();
-  
-        self.end.replace_zero();
+  
+        self.is_iterating = Some(false);
        None
    }
@@ -394,68 +398,24 @@ impl<A: Step> Iterator for ops::RangeInclusive<A> {
    fn max(mut self) -> Option<A> {
        self.next_back()
    }
-  
-
-  
-    #[inline]
-  
-    fn try_fold<B, F, R>(&mut self, init: B, mut f: F) -> R where
-  
-        Self: Sized, F: FnMut(B, Self::Item) -> R, R: Try<Ok=B>
-  
-    {
-  
-        let mut accum = init;
-  
-        if self.start <= self.end {
-  
-            loop {
-  
-                let (x, done) =
-  
-                    if self.start < self.end {
-  
-                        let n = self.start.add_one();
-  
-                        (mem::replace(&mut self.start, n), false)
-  
-                    } else {
-  
-                        self.end.replace_zero();
-  
-                        (self.start.replace_one(), true)
-  
-                    };
-  
-                accum = f(accum, x)?;
-  
-                if done { break }
-  
-            }
-  
-        }
-  
-        Try::from_ok(accum)
-  
-    }
 }
 #[stable(feature = "inclusive_range", since = "1.26.0")]
 impl<A: Step> DoubleEndedIterator for ops::RangeInclusive<A> {
    #[inline]
    fn next_back(&mut self) -> Option<A> {
-  
-        if self.start <= self.end {
-  
-            if self.start < self.end {
-  
-                let n = self.end.sub_one();
-  
-                Some(mem::replace(&mut self.end, n))
-  
-            } else {
-  
-                let last = self.end.replace_zero();
-  
-                self.start.replace_one();
-  
-                Some(last)
-  
-            }
-  
-        } else {
-  
-            None
+  
+        if self.is_empty() {
+  
+            self.is_iterating = Some(false);
+  
+            return None;
        }
-  
-    }
-  
-
-  
-    #[inline]
-  
-    fn try_rfold<B, F, R>(&mut self, init: B, mut f: F) -> R where
-  
-        Self: Sized, F: FnMut(B, Self::Item) -> R, R: Try<Ok=B>
-  
-    {
-  
-        let mut accum = init;
-  
-        if self.start <= self.end {
-  
-            loop {
-  
-                let (x, done) =
-  
-                    if self.start < self.end {
-  
-                        let n = self.end.sub_one();
-  
-                        (mem::replace(&mut self.end, n), false)
-  
-                    } else {
-  
-                        self.start.replace_one();
-  
-                        (self.end.replace_zero(), true)
-  
-                    };
-  
-                accum = f(accum, x)?;
-  
-                if done { break }
-  
-            }
+  
+        if self.start < self.end {
+  
+            let n = self.end.sub_one();
+  
+            self.is_iterating = Some(true);
+  
+            Some(mem::replace(&mut self.end, n))
+  
+        } else {
+  
+            self.is_iterating = Some(false);
+  
+            Some(self.end.clone())
        }
-  
-        Try::from_ok(accum)
    }
 }

diff --git a/src/libcore/ops/range.rs b/src/libcore/ops/range.rs
@@ -9,6 +9,7 @@
 // except according to those terms.
 use fmt;
+  
+use hash::{Hash, Hasher};
 /// An unbounded range (`..`).
 ///
@@ -326,15 +327,37 @@ impl<Idx: PartialOrd<Idx>> RangeTo<Idx> {
 /// assert_eq!(arr[1..=2], [  1,2  ]);  // RangeInclusive
 /// ```
 #[doc(alias = "..=")]
-  
-#[derive(Clone, PartialEq, Eq, Hash)]  // not Copy -- see #27186
+  
+#[derive(Clone)]  // not Copy -- see #27186
 #[stable(feature = "inclusive_range", since = "1.26.0")]
 pub struct RangeInclusive<Idx> {
-  
-    // FIXME: The current representation follows RFC 1980,
-  
-    // but it is known that LLVM is not able to optimize loops following that RFC.
-  
-    // Consider adding an extra `bool` field to indicate emptiness of the range.
-  
-    // See #45222 for performance test cases.
    pub(crate) start: Idx,
    pub(crate) end: Idx,
+  
+    pub(crate) is_iterating: Option<bool>,
+  
+    // This field is:
+  
+    //  - `None` when next() or next_back() was never called
+  
+    //  - `Some(true)` when `start <= end` assuming no overflow
+  
+    //  - `Some(false)` otherwise
+  
+    // The field cannot be a simple `bool` because the `..=` constructor can
+  
+    // accept non-PartialOrd types, also we want the constructor to be const.
+  
+}
+  
+
+  
+#[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
+  
+    }
+  
+}
+  
+
+  
+#[stable(feature = "inclusive_range", since = "1.26.0")]
+  
+impl<Idx: Eq> Eq for RangeInclusive<Idx> {}
+  
+
+  
+#[stable(feature = "inclusive_range", since = "1.26.0")]
+  
+impl<Idx: Hash> Hash for RangeInclusive<Idx> {
+  
+    fn hash<H: Hasher>(&self, state: &mut H) {
+  
+        self.start.hash(state);
+  
+        self.end.hash(state);
+  
+    }
 }
 impl<Idx> RangeInclusive<Idx> {
@@ -350,7 +373,7 @@ impl<Idx> RangeInclusive<Idx> {
    #[stable(feature = "inclusive_range_methods", since = "1.27.0")]
    #[inline]
    pub const fn new(start: Idx, end: Idx) -> Self {
-  
-        Self { start, end }
+  
+        Self { start, end, is_iterating: None }
    }
    /// Returns the lower bound of the range (inclusive).
@@ -492,8 +515,9 @@ impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
    /// assert!(r.is_empty());
    /// ```
    #[unstable(feature = "range_is_empty", reason = "recently added", issue = "48111")]
+  
+    #[inline]
    pub fn is_empty(&self) -> bool {
-  
-        !(self.start <= self.end)
+  
+        !self.is_iterating.unwrap_or_else(|| self.start <= self.end)
    }
 }

diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs
@@ -2262,36 +2262,36 @@ impl<T> SliceIndex<[T]> for ops::RangeInclusive<usize> {
    #[inline]
    fn get(self, slice: &[T]) -> Option<&[T]> {
-  
-        if self.end == usize::max_value() { None }
-  
-        else { (self.start..self.end + 1).get(slice) }
+  
+        if *self.end() == usize::max_value() { None }
+  
+        else { (*self.start()..self.end() + 1).get(slice) }
    }
    #[inline]
    fn get_mut(self, slice: &mut [T]) -> Option<&mut [T]> {
-  
-        if self.end == usize::max_value() { None }
-  
-        else { (self.start..self.end + 1).get_mut(slice) }
+  
+        if *self.end() == usize::max_value() { None }
+  
+        else { (*self.start()..self.end() + 1).get_mut(slice) }
    }
    #[inline]
    unsafe fn get_unchecked(self, slice: &[T]) -> &[T] {
-  
-        (self.start..self.end + 1).get_unchecked(slice)
+  
+        (*self.start()..self.end() + 1).get_unchecked(slice)
    }
    #[inline]
    unsafe fn get_unchecked_mut(self, slice: &mut [T]) -> &mut [T] {
-  
-        (self.start..self.end + 1).get_unchecked_mut(slice)
+  
+        (*self.start()..self.end() + 1).get_unchecked_mut(slice)
    }
    #[inline]
    fn index(self, slice: &[T]) -> &[T] {
-  
-        if self.end == usize::max_value() { slice_index_overflow_fail(); }
-  
-        (self.start..self.end + 1).index(slice)
+  
+        if *self.end() == usize::max_value() { slice_index_overflow_fail(); }
+  
+        (*self.start()..self.end() + 1).index(slice)
    }
    #[inline]
    fn index_mut(self, slice: &mut [T]) -> &mut [T] {
-  
-        if self.end == usize::max_value() { slice_index_overflow_fail(); }
-  
-        (self.start..self.end + 1).index_mut(slice)
+  
+        if *self.end() == usize::max_value() { slice_index_overflow_fail(); }
+  
+        (*self.start()..self.end() + 1).index_mut(slice)
    }
 }

diff --git a/src/libcore/str/mod.rs b/src/libcore/str/mod.rs
@@ -2004,31 +2004,31 @@ mod traits {
        type Output = str;
        #[inline]
        fn get(self, slice: &str) -> Option<&Self::Output> {
-  
-            if self.end == usize::max_value() { None }
-  
-            else { (self.start..self.end+1).get(slice) }
+  
+            if *self.end() == usize::max_value() { None }
+  
+            else { (*self.start()..self.end()+1).get(slice) }
        }
        #[inline]
        fn get_mut(self, slice: &mut str) -> Option<&mut Self::Output> {
-  
-            if self.end == usize::max_value() { None }
-  
-            else { (self.start..self.end+1).get_mut(slice) }
+  
+            if *self.end() == usize::max_value() { None }
+  
+            else { (*self.start()..self.end()+1).get_mut(slice) }
        }
        #[inline]
        unsafe fn get_unchecked(self, slice: &str) -> &Self::Output {
-  
-            (self.start..self.end+1).get_unchecked(slice)
+  
+            (*self.start()..self.end()+1).get_unchecked(slice)
        }
        #[inline]
        unsafe fn get_unchecked_mut(self, slice: &mut str) -> &mut Self::Output {
-  
-            (self.start..self.end+1).get_unchecked_mut(slice)
+  
+            (*self.start()..self.end()+1).get_unchecked_mut(slice)
        }
        #[inline]
        fn index(self, slice: &str) -> &Self::Output {
-  
-            if self.end == usize::max_value() { str_index_overflow_fail(); }
-  
-            (self.start..self.end+1).index(slice)
+  
+            if *self.end() == usize::max_value() { str_index_overflow_fail(); }
+  
+            (*self.start()..self.end()+1).index(slice)
        }
        #[inline]
        fn index_mut(self, slice: &mut str) -> &mut Self::Output {
-  
-            if self.end == usize::max_value() { str_index_overflow_fail(); }
-  
-            (self.start..self.end+1).index_mut(slice)
+  
+            if *self.end() == usize::max_value() { str_index_overflow_fail(); }
+  
+            (*self.start()..self.end()+1).index_mut(slice)
        }
    }

diff --git a/src/test/codegen/issue-45222.rs b/src/test/codegen/issue-45222.rs
@@ -0,0 +1,74 @@
+  
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+  
+// file at the top-level directory of this distribution and at
+  
+// http://rust-lang.org/COPYRIGHT.
+  
+//
+  
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+  
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+  
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+  
+// option. This file may not be copied, modified, or distributed
+  
+// except according to those terms.
+  
+
+  
+// compile-flags: -O
+  
+// min-llvm-version 6.0
+  
+
+  
+#![crate_type = "lib"]
+  
+
+  
+// verify that LLVM recognizes a loop involving 0..=n and will const-fold it.
+  
+
+  
+//------------------------------------------------------------------------------
+  
+// Example from original issue #45222
+  
+
+  
+fn foo2(n: u64) -> u64 {
+  
+    let mut count = 0;
+  
+    for _ in 0..n {
+  
+        for j in (0..=n).rev() {
+  
+            count += j;
+  
+        }
+  
+    }
+  
+    count
+  
+}
+  
+
+  
+// CHECK-LABEL: @check_foo2
+  
+#[no_mangle]
+  
+pub fn check_foo2() -> u64 {
+  
+    // CHECK: ret i64 500005000000000
+  
+    foo2(100000)
+  
+}
+  
+
+  
+//------------------------------------------------------------------------------
+  
+// 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)
+  
+}
+  
+
+  
+//------------------------------------------------------------------------------
+  
+// Demo in #48012
+  
+
+  
+fn foo3r(n: u64) -> u64 {
+  
+    let mut count = 0;
+  
+    (0..n).for_each(|_| {
+  
+        (0 ..= n).rev().for_each(|j| {
+  
+            count += j;
+  
+        })
+  
+    });
+  
+    count
+  
+}
+  
+
+  
+// CHECK-LABEL: @check_foo3r
+  
+#[no_mangle]
+  
+pub fn check_foo3r() -> u64 {
+  
+    // CHECK: ret i64 500005000000000
+  
+    foo3r(100000)
+  
+}