Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-91320: Add _Py_reinterpret_cast() macro#91959
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Fix C++ compiler warnings about "old-style cast"(g++ -Wold-style-cast) in the Python C API. Use C++reinterpret_cast<> and static_cast<> casts when the Python C API isused in C++.Example of fixed warning: Include/object.h:107:43: error: use of old-style cast to ‘PyObject*’ {aka ‘struct _object*’} [-Werror=old-style-cast] #define _PyObject_CAST(op) ((PyObject*)(op))Add _Py_reinterpret_cast() and _Py_static_cast() macros.vstinner commentedApr 26, 2022
serge-sans-paille commentedApr 26, 2022
conceptually looks good to me :-) |
vstinner commentedApr 26, 2022
| #definePY_VECTORCALL_ARGUMENTS_OFFSET ((size_t)1 << (8 * sizeof(size_t) - 1)) | ||
| #definePY_VECTORCALL_ARGUMENTS_OFFSET \ | ||
| (_Py_static_cast(size_t, 1) << (8 * sizeof(size_t) - 1)) |
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.
This macro should be updated since it's used in a static inline function: PyVectorcall_NARGS().
It seems like the warning is emitted if an old-style cast is done in a static inline function, but it seems to be fine if it's only done in macros.
vstinner left a comment
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.
An alternative is to don't add two macros, _Py_static_cast() and _Py_reinterpret_cast(), but only one _Py_CAST() macro which always usesreinterpret_cast<type>(expr) on C++.
vstinner commentedApr 26, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sadly, I used |
corona10 left a comment
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.
looks good to me.
vstinner commentedApr 27, 2022
Thanks for the reviews@serge-sans-paille and@corona10. |
erlend-aasland left a comment
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.
Makes sense; looks good.
tacaswell commentedApr 29, 2022
This suspect that this does not appear to preserve const correctness, I am seeing errors like: when trying to build greenlets, however I have not bisected back to be sure this PR is at fault. |
vstinner commentedApr 30, 2022
The C API requires non-const |
tacaswell commentedApr 30, 2022
My c++ is very rusty, but if the C API requires non-const there failures are correct. The casting to non-const should be done by greenlets (or what ever the calling library is as they should be aware that as escape hatch is being used). I will open an issue with them. |
vstinner commentedMay 1, 2022
Sadly, it's a Python regression compared to Python 3.10. It should be fixed. |
serge-sans-paille commentedMay 1, 2022 via email
You can perform a `const_cast` to by pass the constness check :-/ |
tacaswell commentedMay 1, 2022
Take my view with a grain of 🧂 , but I think you could argue that this was a bug in py310 and below where constness was incorrectly disregarded? Looking at the changes I had to make to greenlet to get it to compile again, I think they are all more "correct" than the code was before. The This is fixing the errors to looked like: Details```In file included from /home/tcaswell/.pybuild/bleeding/include/python3.11/Python.h:38, from src/greenlet/greenlet.cpp:14:src/greenlet/greenlet_refs.hpp: In member function ‘void greenlet::refs::CreatedModule::PyAddObject(const char*, const PyObject*)’:/home/tcaswell/.pybuild/bleeding/include/python3.11/pyport.h:20:44: error: ‘reinterpret_cast’ from type ‘const PyObject*’ {aka ‘const _object*’} to type ‘PyObject*’ {aka ‘_object*’} casts away qualifiers 20 | # define _Py_reinterpret_cast(type, expr) reinterpret_cast(expr) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:107:28: note: in expansion of macro ‘_Py_reinterpret_cast’ 107 | #define _PyObject_CAST(op) _Py_reinterpret_cast(PyObject*, (op)) | ^~~~~~~~~~~~~~~~~~~~/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:505:35: note: in expansion of macro ‘_PyObject_CAST’ 505 | # define Py_INCREF(op) Py_INCREF(_PyObject_CAST(op)) | ^~~~~~~~~~~~~~src/greenlet/greenlet_refs.hpp:854:13: note: in expansion of macro ‘Py_INCREF’ 854 | Py_INCREF(new_object); | ^~~~~~~~~```There is a another class of changes I had to make was to access the wrapped These are the errors that look like: Details |
vstinner commentedMay 2, 2022
PEP 670 was accepted with the condition: conversions must not introduce new compiler warnings (so no new compile errors neither).https://peps.python.org/pep-0670/ |
vstinner commentedMay 2, 2022
We are commenting a closed PR. I created a new issue to discuss the new C++ warning:#92135 |
Fix C++ compiler warnings about "old-style cast"
(g++ -Wold-style-cast) in the Python C API. Use C++
reinterpret_cast<> and static_cast<> casts when the Python C API is
used in C++.
Example of fixed warning:
Add _Py_reinterpret_cast() and _Py_static_cast() macros.