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

gh-98410: move getbufferproc and releasebufferproc to buffer.h #31158

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

davidhewitt
Copy link
Contributor

@davidhewitt davidhewitt commented Feb 6, 2022

After #29991 it is possible to set Py_bf_getbuffer and Py_bf_releasebuffer slots from the stable API.

This PR moves the corresponding typedefs for the function pointer types corresponding to these slots from Include/cpython/object.h to be alongside other typedefs in Include/pybuffer.h.

This may be useful for users who wish to cast to these function pointer types.

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Oct 10, 2022

Closing as the buffer protocol is available in 3.11 in Limited C API.

@davidhewitt
Copy link
Contributor Author

davidhewitt commented Oct 12, 2022

@kumaraditya303 can I please push for you to consider reopening and reviewing this patch? The point that the buffer protocol is available in 3.11 is exactly why I think this should be merged.

As of current 3.11 release on the limited API I can call PyType_GetSlot(Py_TYPE(obj), Py_bf_getbuffer) to get a void * for the getbuffer slot but I can't cast this to a getbufferproc because the definition exists only on the unlimited API. This patch makes getbufferproc available on the limited API.

(And equivalent for releasebufferproc)

@kumaraditya303
Copy link
Contributor

@davidhewitt Sorry for closing your PR. I reopened it and you are correct that the function typedefs are not exposed. I would suggest you to create issue for this issue to discuss this first, nobody looked at this because no issue was mentioned and there are conflicts and and this can't be fixed in 3.11 as the rc is out and final is about to be released.

There are conflicts in this PR, I would suggest you create a new PR with fixed conflicts but it's your choice if you want to continue with this one.

@davidhewitt davidhewitt changed the title move getbufferproc and releasebufferproc to object.h gh-98410: move getbufferproc and releasebufferproc to object.h Oct 18, 2022
@davidhewitt
Copy link
Contributor Author

@kumaraditya303 thanks very much - I have opened #98410 and I'm happy to spend the time to rebase this PR once someone has suggested that a change of this nature would potentially be suitable to merge.

@encukou
Copy link
Member

encukou commented Oct 24, 2022

Thanks for the catch!
Definitely suitable for merge. It won't make it to Python 3.11.0 (sorry!), but it can go in 3.12. (And possibly in 3.11.1, since it's only affecting API rather than ABI.)
Please move the definitions to Include/buffer.h, together with the other buffer API. That way you shouldn't need the forward definition.
Please add these to Misc/stable_abi.toml, as typedef entries added in 3.12, and re-run make regen-limited-abi.

Let me know if you want more info, or if you'd prefer to leave this to me.

@davidhewitt
Copy link
Contributor Author

Thanks - I'll do my best to find a moment to tidy this up tonight!

@davidhewitt davidhewitt changed the title gh-98410: move getbufferproc and releasebufferproc to object.h gh-98410: move getbufferproc and releasebufferproc to buffer.h Oct 25, 2022
@davidhewitt
Copy link
Contributor Author

@encukou how does that look?

@davidhewitt
Copy link
Contributor Author

rebased

@encukou
Copy link
Member

encukou commented Oct 31, 2022

Looks good, thank you!
The rules say these should be available for 3.12+ only, i.e. guarded with:

#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030c0000

but with typedefs, we can relax this rule a little: they'll be available to people building for 3.11 using 3.12.

@encukou encukou merged commit e98923c into python:main Oct 31, 2022
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.

6 participants