Fix itertools.count to take PyNumber instead of PyInt #3822
Conversation
vm/src/stdlib/itertools.rs
Outdated
| #[pyarg(positional, optional)] | ||
| start: OptionalArg<PyIntRef>, |
There was a problem hiding this comment.
#[pyarg] and OptinoalArg is not expected to be removed.
(See the test results how this change broke tests)
Use OptionalArg<PyObjectRef>
vm/src/stdlib/itertools.rs
Outdated
| if let Some(step) = &zelf.step { | ||
| *cur = vm._iadd(&*cur, step.as_object())?; | ||
| } |
There was a problem hiding this comment.
Doesn't 1 have to be added when step is None?
vm/src/stdlib/itertools.rs
Outdated
| stdlib::sys, | ||
| types::{Constructor, IterNext, IterNextIterable}, | ||
| AsObject, Py, PyObjectRef, PyPayload, PyRef, PyResult, PyWeakRef, VirtualMachine, | ||
| }; | ||
| use crossbeam_utils::atomic::AtomicCell; | ||
| use num_bigint::BigInt; | ||
| use num_traits::{One, Signed, ToPrimitive, Zero}; | ||
| // use num_bigint::BigInt; |
There was a problem hiding this comment.
| // use num_bigint::BigInt; |
vm/src/stdlib/itertools.rs
Outdated
| cur: PyRwLock<BigInt>, | ||
| step: BigInt, | ||
| cur: PyRwLock<PyObjectRef>, | ||
| step: Option<PyIntRef>, |
There was a problem hiding this comment.
Can I ask you is there any reason for making step as Option?
And It seems step also need to be checked via PyNumber::check in cpython which means that any type that implements number protocol can be fitted.
There was a problem hiding this comment.
| step: Option<PyIntRef>, | |
| step: PyObjectRef, |
There was a problem hiding this comment.
Because step is an optional value as the second argument of count, set it as Option.
And when processing, there is something similar to repeat, so I considered it.
But if I need to set it up differently, I will.
I checked all reviews. thank. I'll fix.
There was a problem hiding this comment.
What's different between saving it as a PyIntRef and Option<PyIntRef>?
e.g. Does it affect to repr result? If they act same for everything, we don't need to distinguish them.
vm/src/stdlib/itertools.rs
Outdated
| let start = match start.into_option() { | ||
| Some(int) => int.as_bigint().clone(), | ||
| None => BigInt::zero(), | ||
| Some(num) => vm.new_pyobj(num), | ||
| None => vm.new_pyobj(0), | ||
| }; |
There was a problem hiding this comment.
let start: PyObjectRef = start.into_option().unwrap_or_else(|| vm.new_pyobj(0));There was a problem hiding this comment.
I tried but got an error
There was a problem hiding this comment.
Sorry. I was wrong. I'll fix it. Thanks for the suggestion.
vm/src/stdlib/itertools.rs
Outdated
| let step = match step.into_option() { | ||
| Some(int) => int.as_bigint().clone(), | ||
| None => BigInt::one(), | ||
| Some(int) => { | ||
| let val: isize = int.try_to_primitive(vm)?; | ||
| Some(vm.new_pyref(val.to_usize().unwrap_or(0))) | ||
| } | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
let step: PyObjectRef = step.into_option().unwrap_or_else(|| vm.new_pyobj(1));| }; | ||
| if !PyNumber::check(&start, vm) { |
There was a problem hiding this comment.
| if !PyNumber::check(&start, vm) { | |
| if !PyNumber::check(&start, vm) || !PyNumber::check(&step, vm) { |
It seems step also need to be checked via PyNumber::check.
https://github.com/python/cpython/blob/3.10/Modules/itertoolsmodule.c#L4178-L4182
There was a problem hiding this comment.
I didn't because it's a PyIntRef. I'll fix it.
There was a problem hiding this comment.
PyIntRef is always a PyNumber.
vm/src/stdlib/itertools.rs
Outdated
| if let Some(step) = &zelf.step { | ||
| *cur = vm._iadd(&*cur, step.as_object())?; | ||
| } else { | ||
| let one = vm.new_pyobj(1); | ||
| *cur = vm._add(&*cur, &one)?; | ||
| } |
There was a problem hiding this comment.
| if let Some(step) = &zelf.step { | |
| *cur = vm._iadd(&*cur, step.as_object())?; | |
| } else { | |
| let one = vm.new_pyobj(1); | |
| *cur = vm._add(&*cur, &one)?; | |
| } | |
| *cur = vm._add(&*cur, &zelf.step)?; |
If you have interest, this line could be replaced with PyNumber::add which is not yet implemented in rustpython
There was a problem hiding this comment.
that will not be easy before #3819
There was a problem hiding this comment.
I feel like iadd fits better.
| if let Some(step) = &zelf.step { | |
| *cur = vm._iadd(&*cur, step.as_object())?; | |
| } else { | |
| let one = vm.new_pyobj(1); | |
| *cur = vm._add(&*cur, &one)?; | |
| } | |
| vm._iadd(&*cur, step.as_object())?; |
| let result = cur.clone(); | ||
| *cur += &zelf.step; | ||
| *cur = vm._iadd(&*cur, step.as_object())?; |
There was a problem hiding this comment.
is iadd intended to be used with return value?
There was a problem hiding this comment.
oh, it does. I didn't know that.
No description provided.