Skip to content

Add reversed iterator type. #2900

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 2 commits into from
Aug 17, 2021
Merged

Add reversed iterator type. #2900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca6ab79
Add reversed iterator type.
DimitrisJim Aug 17, 2021
fe4c3a5
Merge branch 'master' into reversed_iter_type
DimitrisJim Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion vm/src/builtins/enumerate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
use crate::common::lock::PyRwLock;

use crossbeam_utils::atomic::AtomicCell;
use num_bigint::BigInt;
use num_traits::Zero;

use super::int::PyIntRef;
use super::pytype::PyTypeRef;
use crate::function::OptionalArg;
use crate::iterator;
use crate::slots::PyIter;
use crate::vm::VirtualMachine;
use crate::{iterator, ItemProtocol, TypeProtocol};

@youknowone youknowone Aug 17, 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.

same level as the next line (Line 13)

use crate::{IntoPyObject, PyClassImpl, PyContext, PyObjectRef, PyRef, PyResult, PyValue};

#[pyclass(module = false, name = "enumerate")]
Expand Down Expand Up @@ -60,6 +61,52 @@ impl PyIter for PyEnumerate {
}
}

#[pyclass(module = false, name = "reversed")]
#[derive(Debug)]
pub struct PyReverseSequenceIterator {

@youknowone youknowone Aug 17, 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.

is this belong to enuemerate rather than sequence? I feel like this iterator generate (int, object) rather than object due to the placed module.

@DimitrisJim DimitrisJim Aug 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I know. I don't know why CPython has them in the same file but it does. So I kept the same structure.

pub position: AtomicCell<isize>,

@youknowone youknowone Aug 17, 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.

is this isize rather than a usize?

@DimitrisJim DimitrisJim Aug 17, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Usually the reverse iterators keep an isize in order to check that its > 0 while decreasing it and indexing the underlying object.

pub obj: PyObjectRef,
}

impl PyValue for PyReverseSequenceIterator {
fn class(vm: &VirtualMachine) -> &PyTypeRef {
&vm.ctx.types.reverse_iter_type
}
}

#[pyimpl(with(PyIter))]
impl PyReverseSequenceIterator {
pub fn new(obj: PyObjectRef, len: isize) -> Self {
Self {
position: AtomicCell::new(len - 1),
obj,
}
}

#[pymethod(magic)]
fn length_hint(&self) -> PyResult<isize> {
Ok(self.position.load() + 1)
}
}

impl PyIter for PyReverseSequenceIterator {
fn next(zelf: &PyRef<Self>, vm: &VirtualMachine) -> PyResult {
let pos = zelf.position.fetch_sub(1);

@youknowone youknowone Aug 17, 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.

what happens if this is called forever until negatively overflow?

@DimitrisJim DimitrisJim Aug 17, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm fixing all these methods in another pr. Currently I've just split these appart for now to make the changes a little smaller in size.

@youknowone youknowone Aug 17, 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.

Will you make another huge PR with the whole diff for a reference? I will also check the final result.

@DimitrisJim DimitrisJim Aug 17, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I could push the changes on this branch if it would make it easier for you.

@youknowone youknowone Aug 17, 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.

I think seperated small PR still have benefits. So another PR would be more convenient. It will be reusable after merging this one.

For this PR, I thought I can compare other parts also, but after checking your answers about it, it doesn't look that's nessessary for now.

if pos >= 0 {
match zelf.obj.get_item(pos, vm) {
Err(ref e) if e.isinstance(&vm.ctx.exceptions.index_error) => {
Err(vm.new_stop_iteration())
}
// also catches stop_iteration => stop_iteration
ret => ret,
}
} else {
Err(vm.new_stop_iteration())
}
}
}

pub fn init(context: &PyContext) {
PyEnumerate::extend_class(context, &context.types.enumerate_type);
PyReverseSequenceIterator::extend_class(context, &context.types.reverse_iter_type);
}
38 changes: 9 additions & 29 deletions vm/src/builtins/iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,11 @@ pub enum IterStatus {
Exhausted,
}

#[pyclass(module = false, name = "iter")]
#[pyclass(module = false, name = "iterator")]
#[derive(Debug)]
pub struct PySequenceIterator {
pub position: AtomicCell<isize>,
pub obj: PyObjectRef,
pub reversed: bool,
}

impl PyValue for PySequenceIterator {
Expand All @@ -41,45 +40,26 @@ impl PySequenceIterator {
Self {
position: AtomicCell::new(0),
obj,
reversed: false,
}
}

pub fn new_reversed(obj: PyObjectRef, len: isize) -> Self {
Self {
position: AtomicCell::new(len - 1),
obj,
reversed: true,
}
}

#[pymethod(magic)]
fn length_hint(&self, vm: &VirtualMachine) -> PyResult<isize> {
let pos = self.position.load();
let hint = if self.reversed {
pos + 1
} else {
let len = vm.obj_len(&self.obj)?;
len as isize - pos
};
Ok(hint)
let len = vm.obj_len(&self.obj)?;
Ok(len as isize - pos)
}
}

impl PyIter for PySequenceIterator {
fn next(zelf: &PyRef<Self>, vm: &VirtualMachine) -> PyResult {
let step: isize = if zelf.reversed { -1 } else { 1 };
let pos = zelf.position.fetch_add(step);
if pos >= 0 {
match zelf.obj.get_item(pos, vm) {
Err(ref e) if e.isinstance(&vm.ctx.exceptions.index_error) => {
Err(vm.new_stop_iteration())
}
// also catches stop_iteration => stop_iteration
ret => ret,
let pos = zelf.position.fetch_add(1);
match zelf.obj.get_item(pos, vm) {
Err(ref e) if e.isinstance(&vm.ctx.exceptions.index_error) => {
Err(vm.new_stop_iteration())
}
} else {
Err(vm.new_stop_iteration())
// also catches stop_iteration => stop_iteration
ret => ret,
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions vm/src/builtins/make_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ mod decl {
use crate::builtins::bytes::PyBytesRef;
use crate::builtins::code::PyCodeRef;
use crate::builtins::dict::PyDictRef;
use crate::builtins::enumerate::PyReverseSequenceIterator;
use crate::builtins::function::{PyCellRef, PyFunctionRef};
use crate::builtins::int::{self, PyIntRef};
use crate::builtins::iter::{PyCallableIterator, PySequenceIterator};
use crate::builtins::iter::PyCallableIterator;
use crate::builtins::list::{PyList, SortOptions};
use crate::builtins::pybool::IntoPyBool;
use crate::builtins::pystr::{PyStr, PyStrRef};
Expand Down Expand Up @@ -720,7 +721,7 @@ mod decl {
"argument to reversed() must be a sequence".to_owned()
})?;
let len = vm.obj_len(&obj)? as isize;
let obj_iterator = PySequenceIterator::new_reversed(obj, len);
let obj_iterator = PyReverseSequenceIterator::new(obj, len);
Ok(obj_iterator.into_object(vm))
}
}
Expand Down
2 changes: 2 additions & 0 deletions vm/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ pub struct TypeZoo {
pub generator_type: PyTypeRef,
pub int_type: PyTypeRef,
pub iter_type: PyTypeRef,
pub reverse_iter_type: PyTypeRef,
pub complex_type: PyTypeRef,
pub list_type: PyTypeRef,
pub list_iterator_type: PyTypeRef,
Expand Down Expand Up @@ -182,6 +183,7 @@ impl TypeZoo {
generator_type: generator::PyGenerator::init_bare_type().clone(),
getset_type: getset::PyGetSet::init_bare_type().clone(),
iter_type: iter::PySequenceIterator::init_bare_type().clone(),
reverse_iter_type: enumerate::PyReverseSequenceIterator::init_bare_type().clone(),
list_iterator_type: list::PyListIterator::init_bare_type().clone(),
list_reverseiterator_type: list::PyListReverseIterator::init_bare_type().clone(),
mappingproxy_type: mappingproxy::PyMappingProxy::init_bare_type().clone(),
Expand Down