[cpyrt] Spell out the CPython names behind the Python 2 aliases - #128
Conversation
cpyrt.h still carried the cpyrt_PyText_*, PyInt_*, cpyrt_PyCapsule_* and CPPJIT__*__ aliases that let the sources build against both Python 2 and 3. With Python >= 3.12 required they are plain renames of the PyUnicode_*, PyLong_* and PyCapsule_* APIs and the Python 3 dunder names, so use those directly and drop the aliases, together with the unused cpyrt_PyText_GetSize, PyIntObject, PyBuffer_Type and dict_lookup_func. cpyrt_PyText_AsStringAndSize stays: unlike PyUnicode_AsUTF8AndSize it also accepts bytes. The preprocessed sources are unchanged except in two places. PyObjectDir27.inc tested PyType_Check(obj) || PyClass_Check(obj), which collapses to PyType_Check(obj). CPPJIT_INITIALIZE_STRING stringifies its argument without expanding it, so gDiv held "CPPJIT__div__" rather than "__truediv__"; it now holds "__truediv__". Nothing reads gDiv.
8576567 to
b92f90e
Compare
aaronj0
left a comment
There was a problem hiding this comment.
Great to see that we no longer need the cpyrt_* helpers which were quite annoying when writing CPython extension code. Can you take a look at the concern on PyInt_FromLong -> PyLong_FromLong call sites. Otherwise looks good.
| // special case to be used with arrays: return a Python int instead of str | ||
| // (following the same convention as module array.array) | ||
| return PyInt_FromLong((long)*((signed char*)address)); | ||
| return PyLong_FromLong((long)*((signed char*)address)); |
There was a problem hiding this comment.
I think this special case is supposed to retain the return as PyInt: https://docs.python.org/3/library/array.html
There was a problem hiding this comment.
It was literally defined to be the same in cpyrt.h:
#define PyInt_FromLong PyLong_FromLong
So where is the concern exactly? Maybe you're hinting at something else that is not directly related to this change?
There was a problem hiding this comment.
Oh wow, that looks wrong.. Then yes it's not directly related to your change. Was PyInt_FromLong only in Python2? It just looked like a functional change
There was a problem hiding this comment.
Yes correct, with Python 3 all the PyInt_* functions got replaced by PyLong_* equivalents.
| // special case to be used with arrays: return a Python int instead of str | ||
| // (following the same convention as module array.array) | ||
| return PyInt_FromLong((long)*((unsigned char*)address)); | ||
| return PyLong_FromLong((long)*((unsigned char*)address)); |
| #define cpyrt_PyText_AsString PyUnicode_AsUTF8 | ||
| #define cpyrt_PyText_AsStringChecked PyUnicode_AsUTF8 | ||
| #define cpyrt_PyText_GetSize PyUnicode_GetSize | ||
| #define cpyrt_PyText_GET_SIZE PyUnicode_GET_LENGTH |
There was a problem hiding this comment.
This is a good quality of life improvement :)
There was a problem hiding this comment.
For human and machine 🙂
aaronj0
left a comment
There was a problem hiding this comment.
LGTM! I'd also annotate [NFC] in the commit message when merging
|
CppJIT CI has detected a new failure on workflow Test running on main while building 4ba4a0d at job "macos-26-intel/llvm22/py3.12/c++20 / run". Full details are available at: https://github.com/compiler-research/cppjit/actions/runs/36859698686 |
|
Not arguing against this change. The advantage of the wrappers could be that we have a central place to check for errors of different CRuntime operations and have better diagnostics or ignore logic. |
|
Sure, that's always the advantages of wrappers in general. But that's a whole different topic from having isolated aliases for a random tiny subset from the CPtthon API just because of Python 2 compatibility. |
cpyrt.h still carried the
cpyrt_PyText_*,PyInt_*,cpyrt_PyCapsule_*andCPPJIT__*__aliases that let the sources build against both Python 2 and 3. With Python >= 3.12 required they are plain renames of the PyUnicode_, PyLong_ and PyCapsule_* APIs and the Python 3 dunder names, so use those directly and drop the aliases, together with the unused cpyrt_PyText_GetSize, PyIntObject, PyBuffer_Type and dict_lookup_func.cpyrt_PyText_AsStringAndSize stays: unlike PyUnicode_AsUTF8AndSize it also accepts bytes.
The preprocessed sources are unchanged except in two places. PyObjectDir27.inc tested PyType_Check(obj) || PyClass_Check(obj), which collapses to PyType_Check(obj). CPPJIT_INITIALIZE_STRING stringifies its argument without expanding it, so gDiv held
"CPPJIT__div__"rather than"__truediv__"; it now holds"__truediv__". Nothing reads gDiv.