Skip to content

Implement missing methods for str iterator#2906

Merged
DimitrisJim merged 3 commits into
RustPython:masterfrom
Snowapril:fix-enumeration
Aug 18, 2021
Merged

Implement missing methods for str iterator#2906
DimitrisJim merged 3 commits into
RustPython:masterfrom
Snowapril:fix-enumeration

Conversation

@Snowapril

@Snowapril Snowapril commented Aug 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

This revision implements the missing PyTuple reverse iterator and methods for PyStrIterator and PyReverseIterator

PyTupleReverseIterator

>>>>> t = reversed(tuple('hello'))
>>>>> t.__reduce__()
(<built-in function iter>, (('h', 'e', 'l', 'l', 'o'),), 4)
>>>>> t.__setstate__(2)
>>>>> t.__reduce__()
(<built-in function iter>, (('h', 'e', 'l', 'l', 'o'),), 2)
>>>>> operator.length_hint(t)
3
>>>>> list(t)
['l', 'e', 'h']

PyStrIterator

>>>>> s = 'hello'
>>>>> s = iter('hello')
>>>>> s.__reduce__()
(<built-in function iter>, ('hello',), 0)
>>>>> s.__setstate__(2)
>>>>> s.__reduce__()
(<built-in function iter>, ('hello',), 2)
>>>>> operator.length_hint(s)
3
>>>>> list(s)
['l', 'l', 'o']

PyStrReverseIterator

>>>>> s = reversed('hello')
>>>>> s.__reduce__()
(<built-in function reversed>, ('hello',), 4)
>>>>> s.__setstate__(2)
>>>>> s.__reduce__()
(<built-in function reversed>, ('hello',), 2)
>>>>> operator.length_hint(s)
3
>>>>> list(s)
['l', 'e', 'h']

These changes solve some part of TestReversed.test_len in test_enumerate.py.
I also look into some cpython codes for fixing the rest of failed test in test_len, current implementation of iterator::length_hint need to be replaced by sequence protocol.
Therefore I add trivial comments about it.

@DimitrisJim

DimitrisJim commented Aug 18, 2021

Copy link
Copy Markdown
Member

Unfortunately, both str and tuple don't have a dedicated reverse iterator, they use the generic reversed iterator (see #2900). So those two types aren't necessary here.

@Snowapril

Snowapril commented Aug 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Unfortunately, both str and tuple don't have a dedicated reverse iterator, they use the generic reversed iterator (see #2900). So those two types aren't necessary here.

@DimitrisJim Uh... then, I think this revision could solve some incompatibility with cpython 3.8.
The example codes above are provided in cpython 3.8 but not in current rustpython.
I add more python codes about incompatibility at below.

PyStrIterator

RustPython(mater)

>>>>> a = iter('hello')
>>>>> operator.length_hint(a)
0
>>>>> a.__setstate__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'str_iterator' object has no attribute '__setstate__'
>>>>> a.__reduce__()
(<function _reconstructor at 0x55e7c6094470>, (<class 'str_iterator'>, <class 'object'>, None))

CPython

>>> a = iter('hello')
>>> operator.length_hint(a)
5
>>> a.__setstate__
<built-in method __setstate__ of str_iterator object at 0x7fd2441f53a0>
>>> a.__reduce__()
(<built-in function iter>, ('hello',), 0)

PyStrReverseIterator

RustPython(mater)

>>>>> a = reversed('hello')
>>>>> operator.length_hint(a)
0
>>>>> a.__reduce__()
(<function _reconstructor at 0x55e7c6094470>, (<class 'reversed'>, <class 'object'>, None))
>>>>> a.__setstate__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'str_reverseiterator' object has no attribute '__setstate__'

CPython

>>> a = reversed('hello')
>>> operator.length_hint(a)
5
>>> a.__reduce__()
(<class 'reversed'>, ('hello',), 4)
>>> a.__setstate__
<built-in method __setstate__ of reversed object at 0x7fd2441f53a0>

PyTupleReverseIterator

RustPython(mater)

>>>>> a = reversed(tuple('hello'))
>>>>> operator.length_hint(a)
5
>>>>> a.__setstate__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'reversed' object has no attribute '__setstate__'
>>>>> a.__reduce__()
(<function _reconstructor at 0x55e7c6094470>, (<class 'reversed'>, <class 'object'>, None))

CPython

>>> a = reversed(tuple('hello'))
>>> operator.length_hint(a)
5
>>> a.__setstate__
<built-in method __setstate__ of reversed object at 0x7fd2441ca610>
>>> a.__reduce__()
(<class 'reversed'>, (('h', 'e', 'l', 'l', 'o'),), 4)

@DimitrisJim

DimitrisJim commented Aug 18, 2021

Copy link
Copy Markdown
Member

Oh yes, code for the PyStrIterator is needed.

For the other two, separate types aren't needed. The methods you added can be placed in PyReverseSequenceIterator and the problem will be solved for both reversed(str_instance) and reversed(tuple_instance).

@Snowapril

Snowapril commented Aug 18, 2021

Copy link
Copy Markdown
Contributor Author

Oh. Thank you. I'll send v2 code about it before tomorrow.

@DimitrisJim

DimitrisJim commented Aug 18, 2021

Copy link
Copy Markdown
Member

You could keep the changes for PyStrIterator for now and open a separate PR for the other one when you're done with it.

@Snowapril

Snowapril commented Aug 18, 2021

Copy link
Copy Markdown
Contributor Author

Ok. I dropped two commits without PyStrIterator. I'll open a separate PR for reversed iterators before tomorrow. Thank you.

DimitrisJim
DimitrisJim reviewed Aug 18, 2021
View reviewed changes
Comment thread vm/src/builtins/pystr.rs Outdated
DimitrisJim
DimitrisJim reviewed Aug 18, 2021
View reviewed changes
Comment thread vm/src/builtins/pystr.rs Outdated
@Snowapril
builtins: impl missing methods of iterators in str
93df314 
This commit implements missing methods(`__length_hint__`,
`__setstate__`, `__reduce__`) in `PyStrIterator` and
`PyStrReverseIterator` which cpython 3.8 provided.

For implementing these methods, I add `status: AtomicCell<IterStatus>`.

Implementation referenced on list.rs

Signed-off-by: snowapril <sinjihng@gmail.com>
DimitrisJim
DimitrisJim requested changes Aug 18, 2021
View reviewed changes
Comment thread vm/src/builtins/pystr.rs Outdated
youknowone
youknowone reviewed Aug 18, 2021
View reviewed changes
Comment thread vm/src/builtins/pystr.rs Outdated
use std::{char, ffi, fmt};

use crossbeam_utils::atomic::AtomicCell;

@youknowone youknowone Aug 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@Snowapril Snowapril Aug 18, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed that useless blank line! Thanks for reviewing :)

@youknowone youknowone requested a review from DimitrisJim August 18, 2021 15:46
@DimitrisJim DimitrisJim merged commit 4632972 into RustPython:master Aug 18, 2021
@youknowone youknowone added the z-ca-2021 Tag to track contrubution-academy 2021 label Oct 16, 2021
@youknowone youknowone changed the title Implement missing Iterator & ReverseIterator and its methods Implement missing methods for str iterator Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

@DimitrisJim DimitrisJim DimitrisJim approved these changes

Assignees

No one assigned

Labels

z-ca-2021 Tag to track contrubution-academy 2021

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Snowapril @DimitrisJim @youknowone