fix: address CR problems and fixed size method

killme2008 · killme2008 · commit eed153753932 · 2022-06-12T15:44:09.000+08:00
diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py
@@ -736,8 +736,9 @@ def test_flush_return_value(self):
         mm.write(b'python')
         result = mm.flush()
         self.assertIsNone(result)
-        # TODO: flush doesn't support range righ
-        #if sys.platform.startswith('linux'):
+        # TODO: RUSTPYTHON
+        # Right now memmap2 doesn't throw OSError when offet is not a multiple of mmap.PAGESIZE on Linux
+        # if sys.platform.startswith('linux'):
             # 'offset' must be a multiple of mmap.PAGESIZE on Linux.
             # See bpo-34754 for details.
             # self.assertRaises(OSError, mm.flush, 1, len(b'python'))
diff --git a/stdlib/src/mmap.rs b/stdlib/src/mmap.rs
@@ -15,13 +15,14 @@ mod mmap {
         protocol::{
             BufferDescriptor, BufferMethods, PyBuffer, PyMappingMethods, PySequenceMethods,
         },
-        sliceable::{SaturatedSlice, SequenceIndex},
+        sliceable::{saturate_index, wrap_index, SaturatedSlice, SequenceIndex},
         types::{AsBuffer, AsMapping, AsSequence, Constructor},
         AsObject, FromArgs, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult,
         TryFromBorrowedObject, VirtualMachine,
     };
     use crossbeam_utils::atomic::AtomicCell;
     use memmap2::{Advice, Mmap, MmapMut, MmapOptions};
+    use nix::unistd;
     use std::fs::File;
     use std::io::Write;
     use std::ops::{Deref, DerefMut};
@@ -143,7 +144,7 @@ mod mmap {
     struct PyMmap {
         closed: AtomicCell<bool>,
         mmap: PyMutex<Option<MmapObj>>,
-        _fd: RawFd,
+        fd: RawFd,
         offset: isize,
         size: AtomicCell<isize>,
         pos: AtomicCell<isize>, // relative to offset
@@ -297,7 +298,7 @@ mod mmap {
                     ),
                 )
             } else {
-                let new_fd = nix::unistd::dup(fd).map_err(|e| vm.new_os_error(e.to_string()))?;
+                let new_fd = unistd::dup(fd).map_err(|e| vm.new_os_error(e.to_string()))?;
                 let mmap = match access {
                     AccessMode::Default | AccessMode::Write => MmapObj::Write(
                         unsafe { mmap_opt.map_mut(fd) }
@@ -317,7 +318,7 @@ mod mmap {
             let m_obj = Self {
                 closed: AtomicCell::new(false),
                 mmap: PyMutex::new(Some(mmap)),
-                _fd: fd,
+                fd,
                 offset,
                 size: AtomicCell::new(map_size),
                 pos: AtomicCell::new(0),
@@ -518,28 +519,13 @@ mod mmap {
         fn get_find_range(&self, options: FindOptions) -> (usize, usize) {
             let pos = self.inner_pos();
             let size = self.inner_size();
-            let mut start = options.start.unwrap_or(pos);
-            let mut end = options.end.unwrap_or(size);
+            let start = options.start.unwrap_or(pos);
+            let end = options.end.unwrap_or(size);
 
-            if start < 0 {
-                start += size;
-            }
-            if start < 0 {
-                start = 0;
-            } else if start > size {
-                start = size;
-            }
-
-            if end < 0 {
-                end += size;
-            }
-            if end < 0 {
-                end = 0;
-            } else if end > size {
-                end = size;
-            }
-
-            (start as usize, end as usize)
+            (
+                saturate_index(start, size.try_into().unwrap()),
+                saturate_index(end, size.try_into().unwrap()),
+            )
         }
 
         #[pymethod]
@@ -827,8 +813,15 @@ mod mmap {
         }
 
         #[pymethod]
-        fn size(&self) -> isize {
-            self.inner_size()
+        fn size(&self, vm: &VirtualMachine) -> PyResult<PyIntRef> {
+            let new_fd = unistd::dup(self.fd).map_err(|e| vm.new_os_error(e.to_string()))?;
+            let file = unsafe { File::from_raw_fd(new_fd) };
+            let file_len = match file.metadata() {
+                Ok(m) => m.len(),
+                Err(e) => return Err(vm.new_os_error(e.to_string())),
+            };
+
+            Ok(PyInt::from(file_len).into_ref(vm))
         }
 
         #[pymethod]
@@ -889,15 +882,9 @@ mod mmap {
             Ok(())
         }
 
-        fn get_item_by_index(&self, mut i: isize, vm: &VirtualMachine) -> PyResult<PyObjectRef> {
-            let size = self.inner_size();
-
-            if i < 0 {
-                i += size;
-            }
-            if i < 0 || i >= size {
-                return Err(vm.new_index_error("mmap index out of range".to_owned()));
-            }
+        fn get_item_by_index(&self, i: isize, vm: &VirtualMachine) -> PyResult<PyObjectRef> {
+            let i = wrap_index(i, self.len())
+                .ok_or_else(|| vm.new_index_error("mmap index out of range".to_owned()))?;
 
             let b = match self.check_valid(vm)?.deref().as_ref().unwrap() {
                 MmapObj::Read(mmap) => mmap[i as usize],
@@ -973,18 +960,12 @@ mod mmap {
 
         fn setitem_by_index(
             zelf: PyRef<Self>,
-            mut i: isize,
+            i: isize,
             value: PyObjectRef,
             vm: &VirtualMachine,
         ) -> PyResult<()> {
-            let size = zelf.inner_size();
-
-            if i < 0 {
-                i += size;
-            }
-            if i < 0 || i >= size {
-                return Err(vm.new_index_error("mmap index out of range".to_owned()));
-            }
+            let i = wrap_index(i, zelf.len())
+                .ok_or_else(|| vm.new_index_error("mmap index out of range".to_owned()))?;
 
             zelf.check_writable(vm)?;
             let b = value_from_object(vm, &value)?;
diff --git a/vm/src/sliceable.rs b/vm/src/sliceable.rs
@@ -287,7 +287,7 @@ impl TryFromBorrowedObject for SequenceIndex {
 }
 
 // Use PySliceableSequence::wrap_index for implementors
-pub(crate) fn wrap_index(p: isize, len: usize) -> Option<usize> {
+pub fn wrap_index(p: isize, len: usize) -> Option<usize> {
     let neg = p.is_negative();
     let p = p.wrapping_abs() as usize;
     if neg {

Original file line number	Diff line number	Diff line change
`@@ -287,7 +287,7 @@ impl TryFromBorrowedObject for SequenceIndex {`
`287`	`287`	`}`
`288`	`288`
`289`	`289`	`// Use PySliceableSequence::wrap_index for implementors`
`290`		`-pub(crate) fn wrap_index(p: isize, len: usize) -> Option<usize> {`
	`290`	`+pub fn wrap_index(p: isize, len: usize) -> Option<usize> {`
`291`	`291`	`let neg = p.is_negative();`
`292`	`292`	`let p = p.wrapping_abs() as usize;`
`293`	`293`	`if neg {`