Skip to content

gh-105201: Add PyIter_NextItem() #122331

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

Merged
merged 13 commits into from
Aug 7, 2024

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jul 26, 2024

Return -1 and set an exception on error; return 0 if the iterator is
exhausted, and return 1 if the next item was fetched successfully.

Prefer this API to PyIter_Next(), which requires the caller to use
PyErr_Occurred() to differentiate between iterator exhaution and errors.

Co-authered-by: Irit Katriel [email protected]


📚 Documentation preview 📚: https://cpython-previews--122331.org.readthedocs.build/

Return -1 and set an exception on error; return 0 if the iterator is
exhausted, and return 1 if the next item was fetched successfully.

Prefer this API to PyIter_Next(), which requires the caller to use
PyErr_Occurred() to differentiate between iterator exhaution and errors.

Co-authered-by: Irit Katriel <[email protected]>
@erlend-aasland erlend-aasland requested a review from picnixz July 27, 2024 07:04
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Small nitpicks but those are mostly documentation related. By the way, is there any plan to have something working like next(iter, sentinel) with a sentinel support? or is there some already existing? (or is it even useful for CPython & the universe? I like using next(..., sentinel) when I don't know whether I have an item or not, but I'm not sure whether this pattern has a simple equivalent in CPython).

Copy link
Contributor

@dg-pb dg-pb left a comment

Choose a reason for hiding this comment

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

lgtm

the bug: 'item' was not set to NULL when an incorrect type was given
@erlend-aasland
Copy link
Contributor Author

Thanks for your helpful remarks, everyone. I found a bug and updated the PR. I also noticed we should add the tests to Modules/_testcapi/abstract.c and Lib/test/test_capi/test_abstract.py instead of their current location. I'll update the PR shortly with this adjustment.

@erlend-aasland erlend-aasland requested a review from vstinner July 28, 2024 22:11
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm very sorry for the late review Erland but I was a bit busy this w-e. Those are nitpicking comments, so you can ignore them if you want (just me being compiled in -Wextra -Wpedantic).

@markshannon
Copy link
Member

markshannon commented Jul 29, 2024

The return value should the sole means of determining subsequent control flow, and the output parameter should provide only necessary data, IMO.

In other words, item should only be guaranteed to be set if PyIter_NextItem returns 1, and the control flow in the loop should only test the return value, not item.

Which means the example use needs to rewritten from:

      PyObject *item = NULL;
      while (PyIter_NextItem(iter, &item)) {
          if (item == NULL) {
              Py_DECREF(iter);
              goto error;
          }
          do_something(item);
          Py_DECREF(item);
      }
      Py_DECREF(iter);
      int err;
      while (1) {
          PyObject *item;
          int ret = PyIter_NextItem(iter, &item)) {
          if (err <= 0) break;
          do_something(item);
          Py_DECREF(item);
      }
      Py_DECREF(iter);
      if (err < 0) {
          goto error;
      }

which might be a couple of lines longer, but has only one test in the loop, not two.

We have three options, regarding the value of item if PyIter_NextItem returns <= 0:

  1. Set it to NULL
  2. Do not touch it
  3. Leave it undefined

1 requires work, but adds no additional information.

2 provides options for the user to provide some default and have it unchanged once the iterator exhausted. I don't see much value in this, but it allows a use case that 1 does not.

3 is best for performance and it leaves us free to implement PyIter_NextItem efficiently if the underlying mechanisms change

I'd prefer to specify 3 for now, as it doesn't prevent us from choosing 1 or 2 later on.

@erlend-aasland
Copy link
Contributor Author

We have three options, regarding the value of item if PyIter_NextItem returns <= 0:

The C API WG already decided to go for alternative 1: set it explicitly to NULL.

Which means the example use needs to rewritten from [...]

Actually, I think I'll drop the example from this PR. We can add it in a following PR, if it is needed. I added @zooba's example from the C API WG repo.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@erlend-aasland
Copy link
Contributor Author

Thanks for all the reviews!

@erlend-aasland erlend-aasland merged commit e006c73 into python:main Aug 7, 2024
37 checks passed
@erlend-aasland erlend-aasland deleted the pyiter-nextitem branch August 7, 2024 22:47
@bedevere-bot

This comment was marked as off-topic.

@encukou
Copy link
Member

encukou commented Aug 8, 2024

That failure is unrelated, the iOS buildbot is green now.

blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
Return -1 and set an exception on error; return 0 if the iterator is
exhausted, and return 1 if the next item was fetched successfully.

Prefer this API to PyIter_Next(), which requires the caller to use
PyErr_Occurred() to differentiate between iterator exhaustion and errors.

Co-authered-by: Irit Katriel <[email protected]>
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.

10 participants