-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-87175: Port curses capi pointer array to a struct. #24304
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, this change is backward incompatible. I don't know how to proceed. Is this API used outside Python, in 3rd party projects?
@encukou @serhiy-storchaka @methane: Is this change reasonable?
Note: I asked @shihai1991 to write this change ;-)
The only user I found is this. |
It's a demo and it's commented, so it seems ok. Moreover, the demo uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the incompatible change with a NEWS entry (blurb) and document in C API > Porting to Python 3.10 of:
https://docs.python.org/dev/whatsnew/3.10.html#id2
@methane: Did you check for |
I use https://searchcode.com/ to check who will use it. But I am not find the useful info in here. |
Well, PEP 387 is quite clear on this one, I think. |
Hm, it's more friendly to users. What's your suggestion? victor :) @vstinner |
Why do you want to do this? If we will do that, I think it is safe to replace an array with a structure as long as offsets of structure fields matches offsets of array items. I believe it is true on all supported platforms, because all fields are pointers. |
I proposed this change (and @shihai1991 implemented it) since I don't see a raw array of pointers as an "API".
Even if PyCurses_API_pointers (4) is part of the public Include/py_curses.h header file (and it doesn't exclude anything from the limited C API), it seems like the API is mostly used through these 4 macros:
Not directly through Maybe this API was only exposed for the |
Note: Include/py_curses.h is not included by Python.h. |
(oops, I closed the PR by mistake.) |
This PR is stale because it has been open for 30 days with no activity. |
The capsule object and the related C API is consumed by I'm not sure what it was good idea at the beginning to make such API public. But well, when it was added 23 years ago (commit 0c7a0bd), we cared less about what should be public or private. The C API was mostly consumed by CPython itself. I suggest to make the capsule object private and move the related API to the internal C API. In Python 3.10, I directly removed the |
These APIs are not tested, not documented, and only used by
CPython:
They are not used by any of the PyPI top 5000 projects. cffi has a reference but only in a comment:
|
Thanks for you comment. I am amazing that Victor checked the details again after two years :)
Maybe we can bother this PR's creator to make sure the original purpose. @akuchling I will rebase it tomorrow. |
Could you target |
The least risky approach is to:
|
So 3.12 is prepare to release? I missed a lot of things recently. |
Moreover, its release date was moved (python/peps#3139) from this Monday to May 22. |
No, only the beta (feature freeze). The release is not until this fall. Anyway, I'm not sure how PEP-387 plays with stuff that's not exposed by Python.h. |
Conflicts: Doc/whatsnew/3.10.rst Include/py_curses.h
Hi, I updated this PR(add the internal APIs) and the old APIs in py_curses.h hasn't been removed. So I think I add a note in 3.12 will be ok? |
Probably yes, however, it's up to the core developers to decide.
@erlend-aasland Should we store the new |
It's not common variable in interpreter(just the curses_panel extension module use this Capsule API), so saved in interpreter state is OK. |
I assume you meant module state, no? |
Oh, thanks for correcting my wrong writing. In addition, Use the |
https://bugs.python.org/issue43009