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

Remove special case of PyIterator #1176

Merged
merged 4 commits into from
Oct 12, 2020
Merged

Conversation

birkenfeld
Copy link
Member

Instead of PyIterator<'p> implement the iter protocol on &'p PyIterator.

This lets us implement PyNativeType, pass PyIterator as a function argument etc.

Is there a reason I don't get why this was handled specially?

@davidhewitt
Copy link
Member

Thanks, it's nice to make PyIterator consistent with the rest of the api!

Is there a reason I don't get why this was handled specially?

🤔 yeah I'm not entirely sure of the reason for this either. I have a couple guesses:

  • possibly before the "owned references" design was added to PyO3, PyIterator was already a special case, and nobody else ever noticed it could be updated.
  • I also wonder whether the early drop behaviour of PyIterator<'a> versus &PyIterator is important for some runtime behaviour. But from a quick test just now I couldn't prove that. (And the test suite appears to pass.)

However, I'm of split opinion whether to merge this before releasing 0.12.

Pros:

  • it would simplify the api (which is always awesome imo 🚀)

Cons:

  • we're not entirely sure there wasn't a reason for this (which isn't a reason not to do it, but it could be a reason to wait and think about this a little for 0.13)
  • I think we might be revisiting the owned object design for 0.13 based off discussions in Unexpected memory management behavior #1056, and it might be nice to change the PyIterator API just once in 0.13 rather than once now and once in the next release too.

@davidhewitt
Copy link
Member

I think on balance I'd vote to wait a moment with this PR until 0.12 releases, and then let's merge it on the road to 0.13. That should give us plenty of time to learn if this change was ok - and if we make any bigger changes for 0.13 we can keep everything consistent.

@davidhewitt
Copy link
Member

I rebased on master, added CHANGELOG entry and test. Will merge shortly...

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.

2 participants