GeezerVison is showing github.com — gh-106320: Remove private _PyType_Lookup() function by vstinner · Pull Request #108600 · python/cpython
Conversation
vstinner
commented
Aug 29, 2023
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Move the private function to the internal C API (pycore_object.h).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Move the private function to the internal C API (pycore_object.h).
|
@serhiy-storchaka: I vaguely recall that you considered to expose a similar API to the public C API. Is it still the case? Or what is the PyObject_GetOptionalAttr() API that you added to Python 3.13?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
No, I did not work with _PyType_Lookup(). It seems to me, that _PyType_Lookup() to PyObject_GetAttr() as PyDict_GetItem() to PyObject_GetItem() -- convenient, but flawed by design. And it is not use the descriptor machinery. It is used in many essential parts of the code, and I think that there is a large chance that it is used in third-party projects like Cython. It is difficult if possible to reimplement it with a public API.
Perhaps we should first add a public and better designed variants of _PyType_Lookup() and _PyObject_LookupSpecial() before making them less accessible to third-party developers.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
think that there is a large chance that it is used in third-party projects like Cython
Results of a code search on PyPI top 5,000 projects (2023-07-04).
Affected projects (20):
- Cython (0.29.36)
- GDAL (3.7.0)
- M2Crypto (0.39.0)
- catboost (1.2)
- cvxpy (1.3.2)
- dlib (19.24.2)
- gevent (22.10.2)
- mecab-python3 (1.0.6)
- onnx (1.14.0)
- onnxoptimizer (0.3.13)
- onnxsim (0.4.33)
- osmium (3.6.0)
- praat-parselmouth (0.4.3)
- pybind11 (2.10.4)
- pygraphviz (1.11)
- python-crfsuite (0.9.9)
- pythonnet (3.0.1)
- scipy (1.11.1)
- sentencepiece (0.1.99)
- yappi (1.4.0)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Perhaps we should first add a public
If a private function is widely used and it makes sense to expose it to the public C API, sure, go ahead.
and better designed variants of _PyType_Lookup() and _PyObject_LookupSpecial() before making them less accessible to third-party developers.
Which aspects of the API you would like to change in _PyType_Lookup()?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Which aspects of the API you would like to change in _PyType_Lookup()?
First of all, it contains PyErr_Clear(). We should get rid of such functions.
If it start returning an error, there are variants of the interface: return a pointer with the following calling of PyErr_Occurred() if it is NULL, or return -1/0/1 and save the reference by provided pointer like in PyObject_GetOptionalAttr(), or return a pointer and set the error flag by provided pointer like in find_name_in_mro() and some other private function. It can also return in what type the name was found if there is a need.
Then you may want to return a new reference, although I am not sure that there is a problem with a borrowing reference here. But we should investigate.
It may be turn out that _PyType_Lookup() is too low level, and that all use cases can be covered by few higher level wrappers. Then we can even remove it from internal API.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I close my issue. I prefer to keep _PyType_Lookup() for now.
Well, apparently, it's still interesting to consider adding a public flavor of this API.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|