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 PyDictRef. #810

Merged
merged 4 commits into from
Apr 11, 2019
Merged

Implement IntoIterator for PyDictRef. #810

merged 4 commits into from
Apr 11, 2019

Conversation

cthulahoops
Copy link
Collaborator

Simplify iteration of dictionaries from rust code.

@cthulahoops cthulahoops mentioned this pull request Apr 9, 2019
@@ -247,6 +234,54 @@ impl ItemProtocol for PyDictRef {
}
}

// Implement IntoIterator so that we can easily iterate dictionaries from rust code.
impl IntoIterator for PyDictRef {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than implementing this for the specific type, could this be implemented for &PyDict, and then a blanket impl for PyRef<T> (i.e. something like impl<T> IntoIterator for PyRef<T> where &T: IntoIterator).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good tip. I think we want the other way around: ie. impl<T> IntoIterator for &PyRef<T>, otherwise you always clone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or maybe, not... you mean PyDict not PyDictRef.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But, I don't think you implement it for &PyDict because you need the PyRef in order to clone.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to clone? Why not:

struct DictIter<'a> {
    dict: &'a PyDict,
    ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll give that a go. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get as far as this

impl<T> IntoIterator for PyRef<T>
where
    T: IntoIterator + PyValue,
{
    type Item = <T as IntoIterator>::Item;
    type IntoIter = <T as IntoIterator>::IntoIter;

    fn into_iter(self) -> Self::IntoIter {
        self.deref().into_iter()
    }
}

which gives:

266 |         self.deref().into_iter()
    |         ^^^^^^^^^^^^ cannot move out of borrowed content

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'll merge what I've got here and open a separate PR for this improvement.

@cthulahoops cthulahoops merged commit e04275b into master Apr 11, 2019
@windelbouwman windelbouwman deleted the dict_into_iter branch April 24, 2019 07:53
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.

3 participants