impl and use SequenceIndex for list.__delitem__

Ryex · Ryex · commit 909d94ec2337 · 2019-04-05T20:25:53.000-06:00
- removal of del_item
- all del function impl on PyListRef
- stepped deletion now loops only over the range
  insted of the whole list
diff --git a/tests/snippets/list.py b/tests/snippets/list.py
@@ -190,6 +190,10 @@ def f(x):
 del x[-5:]
 assert x == [1, 2, 3, 4, 5, 6, 7, 8, 10]
 
+x = list(range(12))
+del x[10:2:-2]
+assert x == [0,1,2,3,5,7,9,11]
+
 def bad_del_1():
   del ['a', 'b']['a']
 assert_raises(TypeError, bad_del_1)
diff --git a/vm/src/obj/objlist.rs b/vm/src/obj/objlist.rs
@@ -1,9 +1,10 @@
 use std::cell::{Cell, RefCell};
 use std::fmt;
 
-use std::ops::DerefMut;
+use std::ops::Range;
 
-use num_traits::ToPrimitive;
+use num_bigint::BigInt;
+use num_traits::{One, Signed, ToPrimitive, Zero};
 
 use crate::function::{OptionalArg, PyFuncArgs};
 use crate::pyobject::{IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol};
@@ -13,9 +14,10 @@ use super::objbool;
 use super::objint;
 use super::objiter;
 use super::objsequence::{
-    del_item, get_elements, get_elements_cell, get_item, seq_equal, seq_ge, seq_gt, seq_le, seq_lt,
-    seq_mul, PySliceableSequence,
+    get_elements, get_elements_cell, get_item, seq_equal, seq_ge, seq_gt, seq_le, seq_lt, seq_mul,
+    PySliceableSequence, SequenceIndex,
 };
+use super::objslice::PySliceRef;
 use super::objtype;
 use crate::obj::objtype::PyClassRef;
 
@@ -46,6 +48,54 @@ impl PyValue for PyList {
     }
 }
 
+impl PyList {
+    pub fn get_len(&self) -> usize {
+        self.elements.borrow().len()
+    }
+
+    pub fn get_pos(&self, p: i32) -> Option<usize> {
+        // convert a (potentially negative) positon into a real index
+        if p < 0 {
+            if -p as usize > self.get_len() {
+                None
+            } else {
+                Some(self.get_len() - ((-p) as usize))
+            }
+        } else if p as usize >= self.get_len() {
+            None
+        } else {
+            Some(p as usize)
+        }
+    }
+
+    pub fn get_slice_pos(&self, slice_pos: &BigInt) -> usize {
+        if let Some(pos) = slice_pos.to_i32() {
+            if let Some(index) = self.get_pos(pos) {
+                // within bounds
+                return index;
+            }
+        }
+
+        if slice_pos.is_negative() {
+            // slice past start bound, round to start
+            0
+        } else {
+            // slice past end bound, round to end
+            self.get_len()
+        }
+    }
+
+    pub fn get_slice_range(&self, start: &Option<BigInt>, stop: &Option<BigInt>) -> Range<usize> {
+        let start = start.as_ref().map(|x| self.get_slice_pos(x)).unwrap_or(0);
+        let stop = stop
+            .as_ref()
+            .map(|x| self.get_slice_pos(x))
+            .unwrap_or_else(|| self.get_len());
+
+        start..stop
+    }
+}
+
 pub type PyListRef = PyRef<PyList>;
 
 impl PyListRef {
@@ -313,9 +363,123 @@ impl PyListRef {
         }
     }
 
-    fn delitem(self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult {
+    fn delitem(self, subscript: SequenceIndex, vm: &VirtualMachine) -> PyResult {
+        match subscript {
+            SequenceIndex::Int(index) => self.delindex(index, vm),
+            SequenceIndex::Slice(slice) => self.delslice(slice, vm),
+        }
+    }
+
+    fn delindex(self, index: i32, vm: &VirtualMachine) -> PyResult {
+        if let Some(pos_index) = self.get_pos(index) {
+            self.elements.borrow_mut().remove(pos_index);
+            Ok(vm.get_none())
+        } else {
+            Err(vm.new_index_error("Index out of bounds!".to_string()))
+        }
+    }
+
+    fn delslice(self, slice: PySliceRef, vm: &VirtualMachine) -> PyResult {
+        let start = &slice.start;
+        let stop = &slice.stop;
+        let step = slice.step.clone().unwrap_or_else(BigInt::one);
+
+        if step.is_zero() {
+            Err(vm.new_value_error("slice step cannot be zero".to_string()))
+        } else if step.is_positive() {
+            let range = self.get_slice_range(&start, &stop);
+            if range.start < range.end {
+                #[allow(clippy::range_plus_one)]
+                match step.to_i32() {
+                    Some(1) => {
+                        self._del_slice(range);
+                        Ok(vm.get_none())
+                    }
+                    Some(num) => {
+                        self._del_stepped_slice(range, num as usize);
+                        Ok(vm.get_none())
+                    }
+                    None => {
+                        self._del_slice(range.start..range.start + 1);
+                        Ok(vm.get_none())
+                    }
+                }
+            } else {
+                // no del to do
+                Ok(vm.get_none())
+            }
+        } else {
+            // calculate the range for the reverse slice, first the bounds needs to be made
+            // exclusive around stop, the lower number
+            let start = start.as_ref().map(|x| x + 1);
+            let stop = stop.as_ref().map(|x| x + 1);
+            let range = self.get_slice_range(&stop, &start);
+            if range.start < range.end {
+                match (-step).to_i32() {
+                    Some(1) => {
+                        self._del_slice(range);
+                        Ok(vm.get_none())
+                    }
+                    Some(num) => {
+                        self._del_stepped_slice_reverse(range, num as usize);
+                        Ok(vm.get_none())
+                    }
+                    None => {
+                        self._del_slice(range.end - 1..range.end);
+                        Ok(vm.get_none())
+                    }
+                }
+            } else {
+                // no del to do
+                Ok(vm.get_none())
+            }
+        }
+    }
+
+    fn _del_slice(self, range: Range<usize>) {
+        self.elements.borrow_mut().drain(range);
+    }
+
+    fn _del_stepped_slice(self, range: Range<usize>, step: usize) {
+        // no easy way to delete stepped indexes so here is what we'll do
+        let mut deleted = 0;
+        let mut elements = self.elements.borrow_mut();
+        let mut indexes = range.clone().step_by(step).peekable();
+
+        for i in range.clone() {
+            // is this an index to delete?
+            if indexes.peek() == Some(&i) {
+                // record and move on
+                indexes.next();
+                deleted += 1;
+            } else {
+                // swap towards front
+                elements.swap(i - deleted, i);
+            }
+        }
+        // then drain (the values to delete should now be contiguous at the end of the range)
+        elements.drain((range.end - deleted)..range.end);
+    }
+
+    fn _del_stepped_slice_reverse(self, range: Range<usize>, step: usize) {
+        // no easy way to delete stepped indexes so here is what we'll do
+        let mut deleted = 0;
         let mut elements = self.elements.borrow_mut();
-        del_item(vm, self.as_object(), elements.deref_mut(), key)
+        let mut indexes = range.clone().rev().step_by(step).peekable();
+
+        for i in range.clone().rev() {
+            // is this an index to delete?
+            if indexes.peek() == Some(&i) {
+                // record and move on
+                indexes.next();
+                deleted += 1;
+            } else {
+                // swap towards back
+                elements.swap(i + deleted, i);
+            }
+        }
+        // then drain (the values to delete should now be contiguous at teh start of the range)
+        elements.drain(range.start..(range.start + deleted));
     }
 }
 
diff --git a/vm/src/obj/objsequence.rs b/vm/src/obj/objsequence.rs
@@ -5,7 +5,7 @@ use std::ops::{Deref, DerefMut, Range};
 use num_bigint::BigInt;
 use num_traits::{One, Signed, ToPrimitive, Zero};
 
-use crate::pyobject::{IdProtocol, PyObject, PyObjectRef, PyResult, TypeProtocol};
+use crate::pyobject::{IdProtocol, PyObject, PyObjectRef, PyResult, TryFromObject, TypeProtocol};
 use crate::vm::VirtualMachine;
 
 use super::objbool;
@@ -137,104 +137,21 @@ impl<T: Clone> PySliceableSequence for Vec<T> {
     }
 }
 
-pub trait PySliceableSequenceMut: PySliceableSequence {
-    //fn set_slice<T: PySliceableSequence>(&self, range: Range<usize>, seq: T) -> Self;
-    //fn set_slice_reverse<T: PySliceableSequence>(&self, range: Range<usize>, seq: T) -> Self;
-    //fn set_stepped_slice<T: PySliceableSequence>(&self, range: Range<usize>, step: usize, seq: T) -> Self;
-    //fn set_stepped_slice_reverse<T: PySliceableSequence>(&self, range: Range<usize>, step: usize, seq: T) -> Self;
-
-    fn del_slice(&mut self, range: Range<usize>);
-    fn del_stepped_slice(&mut self, range: Range<usize>, step: usize);
-    fn del_index(&mut self, index: usize);
-
-    fn del_slice_items(&mut self, vm: &VirtualMachine, slice: &PySliceRef) -> PyResult {
-
-        let start = &slice.start;
-        let stop = &slice.stop;
-
-        let step = slice.step.clone().unwrap_or_else(BigInt::one);
-
-        if step.is_zero() {
-            Err(vm.new_value_error("slice step cannot be zero".to_string()))
-        } else if step.is_positive() {
-            let range = self.get_slice_range(start, stop);
-            if range.start < range.end {
-                #[allow(clippy::range_plus_one)]
-                match step.to_i32() {
-                    Some(1) => {
-                        self.del_slice(range);
-                        Ok(vm.get_none())
-                    }
-                    Some(num) => {
-                        self.del_stepped_slice(range, num as usize);
-                        Ok(vm.get_none())
-                    }
-                    None => {
-                        self.del_slice(range.start..range.start + 1);
-                        Ok(vm.get_none())
-                    }
-                }
-            } else {
-                // no del to do
-                Ok(vm.get_none())
-            }
-        } else {
-            // calculate the range for the reverse slice, first the bounds needs to be made
-            // exclusive around stop, the lower number
-            let start = start.as_ref().map(|x| x + 1);
-            let stop = stop.as_ref().map(|x| x + 1);
-            let range = self.get_slice_range(&stop, &start);
-            if range.start < range.end {
-                match (-step).to_i32() {
-                    Some(1) => {
-                        self.del_slice(range);
-                        Ok(vm.get_none())
-                    }
-                    Some(num) => {
-                        self.del_stepped_slice(range, num as usize);
-                        Ok(vm.get_none())
-                    }
-                    None => {
-                        self.del_slice(range.end - 1..range.end);
-                        Ok(vm.get_none())
-                    }
-                }
-            } else {
-                // no del to do
-                Ok(vm.get_none())
-            }
-        }
-           
-    }
+pub enum SequenceIndex {
+    Int(i32),
+    Slice(PySliceRef),
 }
 
-impl<T: Clone> PySliceableSequenceMut for Vec<T> {
-    fn del_slice(&mut self, range: Range<usize>) {
-        self.drain(range);
-    }
-
-    fn del_stepped_slice(&mut self, range: Range<usize>, step: usize) {
-        // no easy way to delete steped indexes so heres what we'll do
-        let len = self.len();
-        let mut deleted = 0;
-        let mut indexes = range.step_by(step).peekable();
-        for i in 0..len {
-            // is this an index to delete?
-            if indexes.peek() == Some(&i) {
-                // record and move on
-                indexes.next();
-                deleted += 1;
-            } else {
-                // swap towards front
-                self.swap(i - deleted, i);
-            }
-        }
-        // then truncate the vec (the values to delete should now be the last elements and thus choped off)
-        self.truncate(len - deleted);
-    }
-
-    fn del_index(&mut self, index: usize) {
-        self.remove(index);
+impl TryFromObject for SequenceIndex {
+    fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
+        match_class!(obj,
+            i @ PyInt => Ok(SequenceIndex::Int(i32::try_from_object(vm, i.into_object())?)),
+            s @ PySlice => Ok(SequenceIndex::Slice(s)),
+            obj => Err(vm.new_type_error(format!(
+                "sequence indices be integers or slices, not {}",
+                obj.class(),
+            )))
+        )
     }
 }
 
@@ -284,47 +201,6 @@ pub fn get_item(
     }
 }
 
-pub fn del_item<T: PySliceableSequenceMut>(
-    vm: &VirtualMachine,
-    sequence: &PyObjectRef,
-    elements: &mut T,
-    subscript: PyObjectRef,
-) -> PyResult {
-    if let Some(i) = subscript.payload::<PyInt>() {
-        return match i.as_bigint().to_i32() {
-            Some(value) => {
-                if let Some(pos_index) = elements.get_pos(value) {
-                    elements.del_index(pos_index);
-                    Ok(vm.get_none())
-                } else {
-                    Err(vm.new_index_error("Index out of bounds!".to_string()))
-                }
-            }
-            None => {
-                Err(vm.new_index_error("cannot fit 'int' into an index-sized integer".to_string()))
-            }
-        };
-    }
-    
-    if subscript.payload::<PySlice>().is_some() {
-        if sequence.payload::<PyList>().is_some() {
-            if let Ok(slice) = subscript.downcast::<PySlice>() {
-                elements.del_slice_items(vm, &slice)
-            } else {
-                panic!("PySlice is not a slice? this should not be")
-            }
-            
-        } else {
-            panic!("sequence del_item called for non-mutable-sequence")
-        }
-    } else {
-        Err(vm.new_type_error(format!(
-            "TypeError: indexing type {:?} with index {:?} is not supported (yet?)",
-            sequence, subscript
-        )))
-    }
-}
-
 pub fn seq_equal(
     vm: &VirtualMachine,
     zelf: &[PyObjectRef],
