add arrayiter.reduce #3868

maong0927 · 2022-07-11T10:59:57Z

No description provided.

Snowapril · 2022-07-11T11:37:24Z

stdlib/src/array.rs


    impl Iterable for PyArray {
        fn iter(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult {
+            let zelf_ = zelf.clone();


Suggested change

let zelf_ = zelf.clone();

youknowone · 2022-07-11T11:36:45Z

stdlib/src/array.rs


    impl Iterable for PyArray {
        fn iter(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult {
+            let zelf_ = zelf.clone();


_ prefix usually means unused variable in rust code.

The _ suffix usually indicates that you're dodging the use of a reserved keyword. From the Rust style guide:

When a name is forbidden because it is a reserved word (e.g., crate), use a trailing underscore to make the name legal (e.g., crate_), or use raw identifiers if possible.

youknowone · 2022-07-11T11:36:58Z

stdlib/src/array.rs

+            let zelf_ = zelf.clone();
            Ok(PyArrayIter {
                position: AtomicUsize::new(0),
                array: zelf,


Suggested change

array: zelf,

array: zelf.clone(),

change here to allow to use zelf later.

Snowapril · 2022-07-11T11:39:04Z

stdlib/src/array.rs

            Ok(PyArrayIter {
                position: AtomicUsize::new(0),
                array: zelf,
+                internal: PyMutex::new(PositionIterInternal::new(zelf_, 0)),


Suggested change

internal: PyMutex::new(PositionIterInternal::new(zelf_, 0)),

internal: PyMutex::new(PositionIterInternal::new(zelf.clone(), 0)),

Is PositionIterInternal necessary for implementing __reduce__?

oh, this is a good point. internal is doing nothing here. it must be position.

The implementation was implemented by referring to the implementation of list iter.
Is there any reason to use zelf as it is instead of zelf.clone in list iter?

I think @Snowapril got confused here. because zelf is already moved in upper line, it can't be cloned here.

Snowapril · 2022-07-12T13:01:11Z

Please do not use git merge. Would you try to rebase it?
https://docs.gitlab.com/ee/topics/git/git_rebase.html

youknowone · 2022-07-12T15:19:46Z

stdlib/src/array.rs

    pub struct PyArrayIter {
        position: AtomicUsize,
        array: PyArrayRef,
+        internal: PyMutex<PositionIterInternal<PyArrayRef>>,


do you really need both position and internal? Though I didn't look in details, those fields look like duplication.

Is it because internal contains position because it is PositionIterInternal as its name suggests?

Check the PositionIterInternal type. it contains position field. By this change, the code have duplicated position fields but most of code uses position and new code uses internal. I don't think it can correctly work

With comparison with other types' iterator, we really don't need position and array field because PyMutex<PositionIterInternal<PyArrayRef>> will do their roles. Because of __next__ and __setstate__, we need to modify position and array fields. It leads to the need for PyMutex.

If we replace position and array fields with PyMutex<PositionIterInternal<PyArrayRef>>, we can implement other methods as usual we did (__next__ and __reduce__)

Does arrayiterator require closed field? I thought it only need position.
closed is useful when it include another iterator in it.

fanninpm · 2022-07-12T16:51:21Z

Please do not use git merge. Would you try to rebase it?
https://docs.gitlab.com/ee/topics/git/git_rebase.html

The following commands should get you started:

git switch main
git pull upstream main
git checkout arrayiter_reduce
git rebase -i main

youknowone · 2022-07-12T17:01:34Z

because people sometimes use main as a working branch, I like to recommend to use upstream/main directly

git fetch upstream
git rebase -i upstream/main

this is same to

git pull --rebase upstream main

Though I never use pull.

maong0927 · 2022-07-13T00:55:58Z

I'll use rebase from now on

Co-authored-by: Yaminyam <siontama@gmail.com>

fanninpm · 2022-07-13T12:57:45Z

According to the CI, there are a few more tests to address. You can run the following command to take a closer look:

cargo run --release --features ssl,jit -- -m test.test_array -vf

In addition, you can run cargo fmt --all to fix the formatting errors.

stdlib/src/array.rs

Co-authored-by: Snowapril <sinjihng@gmail.com>

youknowone

thank you for contributing!

Snowapril reviewed Jul 11, 2022

View reviewed changes

youknowone requested changes Jul 11, 2022

View reviewed changes

Snowapril reviewed Jul 11, 2022

View reviewed changes

fanninpm mentioned this pull request Jul 11, 2022

Implement missing __reduce__ and __reduce_ex__ for several types #3611

Closed

27 tasks

youknowone reviewed Jul 12, 2022

View reviewed changes

youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Jul 13, 2022

maong0927 and others added 3 commits July 13, 2022 18:04

Add arrayiter __reduce__

0f24b18

Co-authored-by: Yaminyam <siontama@gmail.com>

update test_iterator_pickle

ab284c8

Co-authored-by: Yaminyam <siontama@gmail.com>

fix position value to struct value

821f3e7

maong0927 force-pushed the arrayiter_reduce branch from 776a977 to 821f3e7 Compare July 13, 2022 09:12

Fix: Modify iter next to be implemented only internal

1a526af

Snowapril reviewed Jul 16, 2022

View reviewed changes

stdlib/src/array.rs Outdated Show resolved Hide resolved

Yaminyam and others added 4 commits July 16, 2022 16:53

Update stdlib/src/array.rs

50aebef

Co-authored-by: Snowapril <sinjihng@gmail.com>

Feat: impl PyArrayIter.setstate

2417d3e

Refactor: convert indent from tab to space

cdd937c

Refactor: delete unnecessary clones

2bf61aa

youknowone approved these changes Jul 18, 2022

View reviewed changes

youknowone merged commit 4824183 into RustPython:main Jul 18, 2022

	internal: PyMutex::new(PositionIterInternal::new(zelf_, 0)),
	internal: PyMutex::new(PositionIterInternal::new(zelf.clone(), 0)),

add arrayiter.__reduce__ #3868

add arrayiter.__reduce__ #3868

Uh oh!

Conversation

maong0927 commented Jul 11, 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!

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 Jul 12, 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!

Snowapril commented Jul 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Jul 13, 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!

fanninpm commented Jul 12, 2022

Uh oh!

youknowone commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maong0927 commented Jul 13, 2022

Uh oh!

fanninpm commented Jul 13, 2022

Uh oh!

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

add arrayiter.reduce #3868

add arrayiter.reduce #3868

youknowone Jul 12, 2022 •

edited

Loading

youknowone Jul 13, 2022 •

edited

Loading

youknowone commented Jul 12, 2022 •

edited

Loading