Refactor and new sequence traits, generic sequence operation#3445
Conversation
qingshi163
changed the title
| let mul = sequence::seq_mul(vm, &deque, value)?; | ||
| fn _mul(&self, n: isize, vm: &VirtualMachine) -> PyResult<VecDeque<PyObjectRef>> { | ||
| let deque = self.borrow_deque(); | ||
| let n = vm.check_repeat_or_memory_error(deque.len(), n)?; |
There was a problem hiding this comment.
bit confuse, should it return overflow error rather than memory error?
There was a problem hiding this comment.
after reading the issue 45044 from cpython, I think we should replace all the memory error with overflow error because currently we did not catch the malloc failing.
What do you think? @DimitrisJim @youknowone
| #[inline] | ||
| fn _mut_iter_equal_skeleton<F, const SHORT: bool>( |
There was a problem hiding this comment.
This method seems somewhat complex despite it declared as inline. How do you think?
There was a problem hiding this comment.
This one is the skeleton for other related function, only few place is calling it. Also as it is generic, so it should be duplicate for each generic parameters. I mark it inline to try to hint the compiler to optimize out the const bool condition check and the closure it called. I don't know what exactly code the compiler produce but inline is what I expected.
There was a problem hiding this comment.
I don't think #[inline] make any difference for SHORT. const generic paremeter must be a constant regardless of inlining.
| sequence::cmp(vm, a.boxed_iter(), b.boxed_iter(), op).map(PyComparisonValue::Implemented) | ||
| let a = &*zelf.borrow_vec(); | ||
| let b = &*other.borrow_vec(); | ||
| // sequence::cmp(vm, a.boxed_iter(), b.boxed_iter(), op).map(PyComparisonValue::Implemented) |
| if lhs.len() == rhs.len() { | ||
| for (a, b) in lhs.zip_eq(rhs) { | ||
| if !vm.identical_or_equal(a, b)? { | ||
| return Ok(false); | ||
| } | ||
| } | ||
| Ok(true) | ||
| } else { | ||
| Ok(false) | ||
| } |
There was a problem hiding this comment.
| if lhs.len() == rhs.len() { | |
| for (a, b) in lhs.zip_eq(rhs) { | |
| if !vm.identical_or_equal(a, b)? { | |
| return Ok(false); | |
| } | |
| } | |
| Ok(true) | |
| } else { | |
| Ok(false) | |
| } | |
| if lhs.len() != rhs.len() { | |
| return Ok(false); | |
| } | |
| for (a, b) in lhs.zip_eq(rhs) { | |
| if !vm.identical_or_equal(a, b)? { | |
| return Ok(false); | |
| } | |
| } | |
| Ok(true) |
to reduce intent level
| impl SequenceOp<u8> for &str { | ||
| fn as_slice(&self) -> &[u8] { | ||
| self.as_bytes() | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this doesn't fit the convention of as_slice() - when self is str but return type is &[u8].
The only place this is called is PyStr::mul. I'd rather suggest to use zelf.as_str().as_bytes().mul(..) there than implicitly regarding str as sequence of u8.
| impl SequenceOp<u8> for &str { | |
| fn as_slice(&self) -> &[u8] { | |
| self.as_bytes() | |
| } | |
| } |
| #[inline] | ||
| fn _mut_iter_equal_skeleton<F, const SHORT: bool>( |
There was a problem hiding this comment.
I don't think #[inline] make any difference for SHORT. const generic paremeter must be a constant regardless of inlining.
probably by RustPython#3445
3 participants
No description provided.