Fix itertools.count to take PyNumber instead of PyInt by mumallaeng · Pull Request #3822 · RustPython/RustPython

mumallaeng · 2022-06-26T14:29:00Z

No description provided.

youknowone · 2022-06-26T21:06:01Z

-        #[pyarg(positional, optional)]
-        start: OptionalArg<PyIntRef>,


#[pyarg] and OptinoalArg is not expected to be removed.
(See the test results how this change broke tests)

Use OptionalArg<PyObjectRef>

youknowone · 2022-06-26T23:16:32Z

+            if let Some(step) = &zelf.step {
+                *cur = vm._iadd(&*cur, step.as_object())?;
+            }


Doesn't 1 have to be added when step is None?

Snowapril · 2022-06-27T11:07:47Z

    use crossbeam_utils::atomic::AtomicCell;
-    use num_bigint::BigInt;
-    use num_traits::{One, Signed, ToPrimitive, Zero};
+    // use num_bigint::BigInt;


Suggested change

// use num_bigint::BigInt;

Snowapril · 2022-06-27T11:46:01Z

-        cur: PyRwLock<BigInt>,
-        step: BigInt,
+        cur: PyRwLock<PyObjectRef>,
+        step: Option<PyIntRef>,


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.

Suggested change

step: Option<PyIntRef>,

step: PyObjectRef,

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.

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.

Snowapril · 2022-06-27T11:47:05Z

            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),
            };


let start: PyObjectRef = start.into_option().unwrap_or_else(|| vm.new_pyobj(0));

I tried but got an error

Sorry. I was wrong. I'll fix it. Thanks for the suggestion.

Snowapril · 2022-06-27T11:47:24Z

            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,
            };


let step: PyObjectRef = step.into_option().unwrap_or_else(|| vm.new_pyobj(1));

Snowapril · 2022-06-27T11:48:40Z

+                }
+                None => None,
            };
+            if !PyNumber::check(&start, vm) {


Suggested change

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

I didn't because it's a PyIntRef. I'll fix it.

PyIntRef is always a PyNumber.

Snowapril · 2022-06-27T11:50:14Z

+            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)?;
+            }


Suggested change

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

that will not be easy before #3819

I feel like iadd fits better.

Suggested change

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())?;

youknowone · 2022-06-27T13:46:58Z

+            let step = zelf.step.clone();
            let result = cur.clone();
-            *cur += &zelf.step;
+            *cur = vm._iadd(&*cur, step.as_object())?;


is iadd intended to be used with return value?

oh, it does. I didn't know that.

youknowone

Looks good, thank you for contributing!

Fix to get number of itertools count

aad60e8

youknowone reviewed Jun 26, 2022

View reviewed changes

mumallaeng added 2 commits June 27, 2022 18:14

Fix itertools count

79cb376

Fix itertools count

d3485f4

Snowapril reviewed Jun 27, 2022

View reviewed changes

Fix itertools count

e7c1eb2

Snowapril reviewed Jun 27, 2022

View reviewed changes

mumallaeng added 2 commits June 27, 2022 21:50

Fix itertools count step

89ae703

Fix itertools count PyResult

4422a54

youknowone changed the title ~~Fix to get number of itertools count~~ Fix itertools.count to take PyNumber instead of PyInt Jun 27, 2022

mumallaeng added 2 commits June 27, 2022 22:05

Fix itertools count next

e1212b7

Fix clippy error

fd85e71

youknowone reviewed Jun 27, 2022

View reviewed changes

youknowone approved these changes Jun 27, 2022

View reviewed changes

youknowone merged commit 6ad0e54 into RustPython:main Jun 27, 2022

mumallaeng deleted the iter_count_get_number branch July 17, 2022 23:28

	if !PyNumber::check(&start, vm) {
	if !PyNumber::check(&start, vm) \|\| !PyNumber::check(&step, vm) {

Conversation

mumallaeng commented Jun 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Snowapril Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Snowapril Jun 27, 2022 •

edited

Loading