Skip to content
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.

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

Implement IntoIterator for PySet and PyFrozenSet #716

Merged
merged 2 commits into from
Jan 8, 2020

Conversation

davidhewitt
Copy link
Member

Closes #715

@kngwyu
Copy link
Member

kngwyu commented Jan 8, 2020

Could you please rewrite the implementation using _PySet_NextEntry?

  1. Add this declaration in ffi::setobject.rs
#[cfg(not(Py_LIMITED_API))]
extern "C" {
    #[cfg_attr(PyPy, link_name = "_PySet_NextEntry")]
    pub fn _PySet_NextEntry(
        set: *mut PyObject,
        pos: *mut Py_ssize_t,
        key: *mut *mut PyObject,
        hash: *mut super::Py_hash_t,
    ) -> c_int;
}
  1. Then implementation comes like
#[cfg(not(Py_LIMITED_API))]
pub struct PySetIterator<'py> {
    set: &'py super::PyAny,
    pos: isize,
}

#[cfg(not(Py_LIMITED_API))]
impl<'py> Iterator for PySetIterator<'py> {
    type Item = &'py super::PyAny;

    #[inline]
    fn next(&mut self) -> Option<Self::Item> {
        unsafe {
            let mut key: *mut ffi::PyObject = std::ptr::null_mut();
            let mut hash: ffi::Py_hash_t = 0;
            if ffi::_PySet_NextEntry(self.set.as_ptr(), &mut self.pos, &mut key, &mut hash) != 0 {
                Some(self.set.py().from_borrowed_ptr(key))
            } else {
                None
            }
        }
    }
}

#[cfg(not(Py_LIMITED_API))]
impl<'a> std::iter::IntoIterator for &'a PySet {
    type Item = &'a PyAny;
    type IntoIter = PySetIterator<'a>;

    fn into_iter(self) -> Self::IntoIter {
        PySetIterator {
            set: self.as_ref(),
            pos: 0,
        }
    }
}

@davidhewitt
Copy link
Member Author

👍 Thanks - I missed _PySet_Next because I didn't find it at https://docs.python.org/3/c-api/set.html!

@kngwyu
Copy link
Member

kngwyu commented Jan 8, 2020

Thank you!

@kngwyu kngwyu merged commit b3ca27d into PyO3:master Jan 8, 2020
@samuelcolvin
Copy link
Contributor

Thanks for fixing this.

My only concern is that PySet still doesn't implement iter() as an explicit method (i'm not sure how that's being implemented, but I guess it must be as it's working in tests) so it's not included in the documentation.

For a list (ignoring the comment that I guess is left over from copying the method from tuples) we have:

pyo3/src/types/list.rs

Lines 116 to 122 in b3ca27d

/// Returns an iterator over the tuple items.
pub fn iter(&self) -> PyListIterator {
PyListIterator {
list: self,
index: 0,
}
}

Would it help to add iter() to sets? It's trivial enough that even I can create a PR.

@davidhewitt davidhewitt deleted the set-iterator branch January 8, 2020 11:31
@davidhewitt
Copy link
Member Author

I left .iter() off because I didn't see a need for it, given IntoIterator is now implemented and that'll be in the docs.

If you still would like to have iter(), please do open a PR. Note you'd need to wrap any iter() method you add in #[cfg(not(Py_LIMITED_API))], like the other definitions we just merged.

@samuelcolvin
Copy link
Contributor

okay, I'll leave it.

@kngwyu
Copy link
Member

kngwyu commented Jan 8, 2020

@samuelcolvin
I implemented it in #717, for the compatibility with PyDict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PySet is not iterable
3 participants