gh-91320: Add _Py_reinterpret_cast() macro #91959
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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:
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.
|
conceptually looks good to me :-) |
vstinner
commented
Apr 26, 2022
| @@ -50,7 +50,8 @@ PyAPI_FUNC(PyObject *) _PyObject_MakeTpCall( | |||
| PyObject *const *args, Py_ssize_t nargs, | |||
| PyObject *keywords); | |||
|
|
|||
| #define PY_VECTORCALL_ARGUMENTS_OFFSET ((size_t)1 << (8 * sizeof(size_t) - 1)) | |||
| #define PY_VECTORCALL_ARGUMENTS_OFFSET \ | |||
| (_Py_static_cast(size_t, 1) << (8 * sizeof(size_t) - 1)) | |||
There was a problem hiding this comment.
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
commented
Apr 26, 2022
Sadly, I used |
|
Thanks for the reviews @serge-sans-paille and @corona10. |
|
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. |
The C API requires non-const |
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. |
This was referenced Apr 30, 2022
|
Sadly, it's a Python regression compared to Python 3.10. It should be fixed. |
|
You can perform a `const_cast` to by pass the constness check :-/
|
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:
```
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: |
|
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/ |
|
We are commenting a closed PR. I created a new issue to discuss the new C++ warning: #92135 |
This was referenced May 14, 2022
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.