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

Add HPyIter_(Check|Next) and HPy_GetIter #476

Merged
merged 5 commits into from
Mar 1, 2024

Conversation

Sachaa-Thanasius
Copy link
Contributor

@Sachaa-Thanasius Sachaa-Thanasius commented Feb 20, 2024

I was giving HPy a try and got confused on how to validate an iterable as a function input. Figured this would be one way to validate such an object and cycle through its members. The tests I added feel rough and I haven't added any docs yet (excluding anything autogenerated); regardless, any feedback would be appreciated.

@Sachaa-Thanasius Sachaa-Thanasius changed the title Add HPy_Iter(Check|Next) and HPyObject_GetIter Add HPyIter_(Check|Next) and HPy_GetIter Feb 20, 2024
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Thanks. This is looking great so far. What are the parts you'd still like to add?

It would be nice to add documentation for the three new functions to public_api.h if you have the time.

@Sachaa-Thanasius
Copy link
Contributor Author

Well, something I'm not sure about is scope for this PR. Is this enough on its own, or would make sense to uncomment HPy_tp_iter and HPy_tp_iternext and take care of those as well? What about adding equivalents to PyAIter_Check and PyIter_Send? Does the fact that some of these functions were only added to CPython's stable ABI after 3.8 or 3.10 matter? I only added what I personally wanted based on my use case, so I don't know if anything else is needed.

As for function docstrings, sure. I'll add those, using the surrounding ones as reference.

@hodgestar
Copy link
Contributor

It's definitely enough by itself, and small PRs are nice if one can have them.

Regarding the other parts:

  • HPy_tp_iter and HPy_tp_iternext: I would have put these into a separate PR in any case since they're for implementing iterators and iterables rather than using them.
  • PyAIter_Check and PyIter_Send: If the HPy versions can be implemented on Python 3.8+ then they can be included regardless of whether they're part of the C API for some version of Python.

- Move functions to be consistent with reference file comments (i.e. `abstract.h`).
- Additionally, add a bit more body to the HPyIter_Next test method.
@Sachaa-Thanasius
Copy link
Contributor Author

Okay, that makes sense. I think that if I contribute anything more for the iterator-related API, it'll be in another PR.

I've added docstrings and moved the functions to have more consistent placement within public_api.h (based on the comments referencing CPython files where the counterparts exist). Anything else needed?

@Sachaa-Thanasius Sachaa-Thanasius marked this pull request as ready for review February 20, 2024 21:58
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

One small documentation fix suggestion, but otherwise looks good to me.

@mattip @fangerer (and anyone else): Would you mind having a quick look and checking that this doesn't conflict with any of your plans or make things more difficult for the PyPy and GraalPy implementations?

hpy/tools/autogen/public_api.h Outdated Show resolved Hide resolved
Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
@steve-s
Copy link
Contributor

steve-s commented Feb 29, 2024

lgtm. I cannot think about any implementation issues this would cause on GraalPy side or in general. Let's give it a day in case anyone else wants to comment and I'd merge this PR. Thanks @Sachaa-Thanasius for the contribution!

@steve-s steve-s merged commit 95c51b9 into hpyproject:master Mar 1, 2024
38 checks passed
@Sachaa-Thanasius Sachaa-Thanasius deleted the feature/add_hpy_iter branch March 2, 2024 03:03
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