-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
PyIndex_Check conflicts with PEP 384 #77919
Comments
The file number.rst on python 3.6 says """ Returns But in reality, this is a macro: """
#define PyIndex_Check(obj) \
((obj)->ob_type->tp_as_number != NULL && \
(obj)->ob_type->tp_as_number->nb_index != NULL)
""" But such a macro does not work with the limited API, since The patch is trivial: Define the function instead of the macro I also think the documentation makes more sense when it is correct. |
This is not a security issue so a change would not be applicable to the 3.4 or 3.5 branches, currently in security-fix-only mode. |
Seems PyIter_Check and PySequence_ITEM have the same problem. And maybe more. |
Ok, I tried to submit a patch (not yet successful), Should I take care of these all as much as I can, Whatever you prefer is ok. I just want the problem to disappear Cheers - Chris |
Would be nice if you fix as much similar as you can with a single PR. For performance it makes sense to keep macros if the limited API is not used. |
Yes, sure, I will submit a patch that tries to reach as much Takes a day because I need to re-learn a bit how to do this :-) -- related personal note - does someone know what happened to Martin? |
I don't think anything bad happened. He still seems to be teaching: |
Christian, any progress on this? 3.7.0rc1 is planned to be tagged in 4 days, on Monday 2018-06-11. Do you think you will be able to provide a PR before then? |
Hi Ned, we had a delivery date yesterday for PySide. Ciao - Chris On 07.06.18 09:43, Ned Deily wrote:
|
Thanks, Christian! |
I did not understand the Misc/NEWS.d thing. |
@christian, you can use the "blurb" tool to create the NEWS entry. You can use pip to install it. See: https://devguide.python.org/committing/?highlight=blurb#what-s-new-and-news-entries |
On 07.06.18 17:59, Eric Snow wrote:
Thank you, Eric! Will do so. |
Excluding names from limited API can break existing code that use them with defined Py_LIMITED_API. I wondering if corresponding functions should be added for PySequence_ITEM, PyObject_IS_GC, PyType_SUPPORTS_WEAKREFS, PyObject_GET_WEAKREFS_LISTPTR. Perhaps this should be discussed on Python-Dev. Since PyList_GET_SIZE and PyList_GET_ITEM are defined only for non-limited API, it is better to wrap definitions of macros that use them (like PySequence_Fast_GET_SIZE and PySequence_Fast_GET_ITEM) in "#ifndef Py_LIMITED_API" ... "#endif". |
"""Excluding names from limited API can break existing code that use them with defined Py_LIMITED_API.""" How is that different? Right now, the code would break at compile time, My current first approach is conservative, because it only makes things Right now I want to remove quickly the breakage that was a showstopper. Please let us start a discussion on python-dev. I think there is more to |
As I noted on PR 7477, I decided to merge the change to master so it would get some buildbot exposure before deciding whether to backport for 3.7.0rc1 which is coming up shortly. I notice that the change introduced a compiler warning. Should that be fixed? ..\Objects\exceptions.c(349): warning C4090: 'return': different 'const' qualifiers [D:\buildarea\3.x.ware-win81-release\build\PCbuild\pythoncore.vcxproj] |
You are right Christian. I missed that PyTypeObject is opaque if Py_LIMITED_API is defined. |
It seems like the change broke compilation on Windows, but I'm not sure: http://buildbot.python.org/all/#/builders/12/builds/959
On the buildbot-status, Ned Deily wrote: Build failure caused by known failure to rebuild files during compile phase. Worked around by clearing out stale files. (I *think* there is still an open issue on this.) |
Sigh! I was hoping we could get this in for 3.7.0 but I think we have run out of time and we really should not be making potential user-visible API changes at this last minute. I did notice the new compile warning for the Windows non-debug build but I overlooked that some other non-Windows buildbots were getting them, too. Serihy proposes a more general fix in bpo-33818 but I don't think we should be making a change like that just prior to the release candidate. And, in retrospect, I should not have considered trying to fix the stable ABI support at this late date, either. It looks like it has been broken for some time now so 3.7.0 will not be any worse. At this point, we have Christian's two PRs in master now; if necessary, they could be reverted. I will bow out of this discussion and let you all figure out what is best for master/3.8. Once the changes for master are in and working, we could revisit the question of backports to 3.7 and/or 3.6 maintenance releases. I would like to both thank and apologize to Christian, in particular, and to Serhiy and everyone else who went out of their ways to try to get this in. Lowering priority to "critical". |
AMD64 Windows8.1 Non-Debug 3.x buildbot is back to green. |
@serhiy, @christian: Is there anything more needed to be done for this issue or can we close it? |
Ned:
The issue is fixed in Python 3.8, I close it. See also bpo-40170 "[C API] Make PyTypeObject structure an opaque structure in the public C API" which goes further. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: