Skip to content

Chain reduce #4232

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 13 commits into from
Oct 21, 2022
Merged

Chain reduce #4232

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e91ac80
add reduce for chain
dannasman Oct 18, 2022
108519a
Merge branch 'main' of github.com:dannasman/RustPython into chain_reduce
dannasman Oct 18, 2022
05ce6f8
fix formatting issues
dannasman Oct 18, 2022
7aff198
change some clone() calls to to_owned() to fix mismatched type errors
dannasman Oct 18, 2022
bda0f4e
fix formatting issues
dannasman Oct 18, 2022
1e4e266
add HAS_DICT flag to chain
dannasman Oct 18, 2022
c398635
fix formatting issues
dannasman Oct 18, 2022
9609b2e
fix clippy issues
dannasman Oct 18, 2022
33de7b7
remove reduce_ex for now
dannasman Oct 18, 2022
1f38cd3
add setstate for chain
dannasman Oct 19, 2022
60c295a
Merge branch 'RustPython:main' into chain_reduce
dannasman Oct 20, 2022
d0b5654
switch back the to_owned() calls and remove flag changes (see commit …
dannasman Oct 20, 2022
e32d4de
fix styling and lock issues
dannasman Oct 21, 2022
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
5 changes: 1 addition & 4 deletions Lib/test/test_itertools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,6 @@ def test_chain_from_iterable(self):
self.assertEqual(take(4, chain.from_iterable(['abc', 'def'])), list('abcd'))
self.assertRaises(TypeError, list, chain.from_iterable([2, 3]))

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_chain_reducible(self):
for oper in [copy.deepcopy] + picklecopiers:
it = chain('abc', 'def')
Expand All @@ -195,8 +193,7 @@ def test_chain_reducible(self):
self.assertRaises(TypeError, list, oper(chain(2, 3)))
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
self.pickletest(proto, chain('abc', 'def'), compare=list('abcdef'))
# TODO: RUSTPYTHON
@unittest.expectedFailure

def test_chain_setstate(self):
self.assertRaises(TypeError, chain().__setstate__, ())
self.assertRaises(TypeError, chain().__setstate__, [])
Expand Down
47 changes: 46 additions & 1 deletion vm/src/stdlib/itertools.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ mod decl {
active: PyRwLock<Option<PyIter>>,
}

#[pyclass(with(IterNext), flags(BASETYPE))]
#[pyclass(with(IterNext), flags(BASETYPE, HAS_DICT))]
impl PyItertoolsChain {
#[pyslot]
fn slot_new(cls: PyTypeRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult {
Expand Down Expand Up @@ -59,6 +59,51 @@ mod decl {
fn class_getitem(cls: PyTypeRef, args: PyObjectRef, vm: &VirtualMachine) -> PyGenericAlias {
PyGenericAlias::new(cls, args, vm)
}

#[pymethod(magic)]
fn reduce(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult<PyTupleRef> {
let source = zelf.source.read().clone();
let active = zelf.active.read().clone();
let cls = zelf.class().to_owned();
match source {
Some(source) => match active {
Some(active) => {
Ok(vm.new_tuple((cls, vm.ctx.empty_tuple.clone(), (source, active))))
}
None => Ok(vm.new_tuple((cls, vm.ctx.empty_tuple.clone(), (source,)))),
},
None => Ok(vm.new_tuple((cls, vm.ctx.empty_tuple.clone()))),
}

@youknowone youknowone Oct 21, 2022

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
match source {
Some(source) => match active {
Some(active) => {
Ok(vm.new_tuple((cls, vm.ctx.empty_tuple.clone(), (source, active))))
}
None => Ok(vm.new_tuple((cls, vm.ctx.empty_tuple.clone(), (source,)))),
},
None => Ok(vm.new_tuple((cls, vm.ctx.empty_tuple.clone()))),
}
let empty_tuple = vm.ctx.empty_tuple.clone();
let reduced = match source {
Some(source) => match active {
Some(active) => vm.new_tuple((cls, empty_tuple, (source, active))),
None => vm.new_tuple((cls, empty_tuple, (source,))),
},
None => vm.new_tuple((cls, empty_tuple)),
};
Ok(reduced)

}

#[pymethod(magic)]
fn setstate(zelf: PyRef<Self>, state: PyTupleRef, vm: &VirtualMachine) -> PyResult<()> {
let args = state.as_slice();
if args.is_empty() {
let msg = String::from("function takes at leat 1 arguments (0 given)");
return Err(vm.new_type_error(msg));
}
if args.len() > 2 {
let msg = format!("function takes at most 2 arguments ({} given)", args.len());
return Err(vm.new_type_error(msg));
}
let source = &args[0];
if args.len() == 1 {
if !PyIter::check(source.as_ref()) {
return Err(vm.new_type_error(String::from("Arguments must be iterators.")));
}
*zelf.source.write() = source.to_owned().try_into_value(vm)?;
return Ok(());
}
let active = &args[1];

if !PyIter::check(source.as_ref()) || !PyIter::check(active.as_ref()) {
return Err(vm.new_type_error(String::from("Arguments must be iterators.")));
}
*zelf.source.write() = source.to_owned().try_into_value(vm)?;
*zelf.active.write() = active.to_owned().try_into_value(vm)?;

@youknowone youknowone Oct 21, 2022 •
edited
Loading

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.

To avoid unpleasure broken synchronization, locking both and writing will be better.

Suggested change
*zelf.source.write() = source.to_owned().try_into_value(vm)?;
*zelf.active.write() = active.to_owned().try_into_value(vm)?;
let mut source_lock = zelf.source.write();
let mut active_lock = zelf.active.write();
*source_lock = source.to_owned().try_into_value(vm)?;
*active_lock = active.to_owned().try_into_value(vm)?;

Ok(())
}
}
impl IterNextIterable for PyItertoolsChain {}
impl IterNext for PyItertoolsChain {
Expand Down