- Notifications
You must be signed in to change notification settings - Fork1.3k
Refactor and new sequence traits, generic sequence operation#3445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vm/src/stdlib/collections.rs Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bit confuse, should it return overflow error rather than memory error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that sounds like reasonable
vm/src/sequence.rs Outdated
| #[inline] | ||
| fn _mut_iter_equal_skeleton<F, const SHORT: bool>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This method seems somewhat complex despite it declared asinline. How do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think#[inline] make any difference forSHORT. const generic paremeter must be a constant regardless of inlining.
vm/src/builtins/list.rs Outdated
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
outdated comment?
vm/src/sequence.rs Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if lhs.len() == rhs.len(){ | |
| for(a, b)in lhs.zip_eq(rhs){ | |
| if !vm.identical_or_equal(a, b)?{ | |
| returnOk(false); | |
| } | |
| } | |
| Ok(true) | |
| }else{ | |
| Ok(false) | |
| } | |
| if lhs.len() != rhs.len(){ | |
| returnOk(false); | |
| } | |
| for(a, b)in lhs.zip_eq(rhs){ | |
| if !vm.identical_or_equal(a, b)?{ | |
| returnOk(false); | |
| } | |
| } | |
| Ok(true) |
to reduce intent level
vm/src/sequence.rs Outdated
| impl SequenceOp<u8> for &str { | ||
| fn as_slice(&self) -> &[u8] { | ||
| self.as_bytes() | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this doesn't fit the convention ofas_slice() - whenself isstr but return type is&[u8].
The only place this is called isPyStr::mul. I'd rather suggest to usezelf.as_str().as_bytes().mul(..) there than implicitly regarding str as sequence of u8.
| implSequenceOp<u8>for&str{ | |
| fn as_slice(&self) ->&[u8]{ | |
| self.as_bytes() | |
| } | |
| } |
vm/src/sequence.rs Outdated
| #[inline] | ||
| fn _mut_iter_equal_skeleton<F, const SHORT: bool>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think#[inline] make any difference forSHORT. const generic paremeter must be a constant regardless of inlining.
No description provided.