Add HPyIter_(Check|Next) and HPy_GetIter

hpyproject / hpy

HPy: a better API for Python

https://hpyproject.org

MIT License

1.02k stars 52 forks source link

Add HPyIter_(Check|Next) and HPy_GetIter #476

Closed Sachaa-Thanasius closed 4 months ago

Sachaa-Thanasius commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

Sachaa-Thanasius commented 4 months ago

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?

steve-s commented 4 months ago

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!