-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-117549: Match declaration order for _Py_BackoffCounter initializer #117551
Conversation
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.
LGTM. Let's wait for @tacaswell to give us a thumbs up here before we merge.
I can confirm that scipy compiles with cpython from this branch. |
Thanks for your help! Merging now... |
…alizer (python#117551) Otherwise it might not compile with C++ (or certain C compilers/flags?).
Cython includes this file and g++ 9 and 10 (available on Ubuntu 22.04 LTS) doesn't compile this. msgpack/msgpack-python#593 (comment) // foo.cpp
#define Py_BUILD_CORE 1
#include <Python.h>
#include "frameobject.h"
#include "internal/pycore_frame.h"
int main() {}
// g++-10 -std=c++17 -I include/python3.13 foo.cpp I confirmed Using temporary variable fix it. _Py_BackoffCounter c;
c.backoff = backoff;
c.value = value;
return c; But I am not sure we should support g++ 9/10 in Python 3.13. |
I am despairing. We make all these efforts to indicate things are internal and they are still included in half the packages in the world. :-( |
FYI, this caused compile error on MSVC too. Should we add |
Wait— if it failed on MSVC how could the original code have passed CI? |
Because we don't test C++. |
|
So let’s fix it. Do you have time for a PR? |
That should be fixed now. |
thank you. |
By the way, Cython uses this only for building custom traceback object. Adding https://github.com/cython/cython/blob/45db483cbe128c29a1e9ffe9f5a4db12cf77cddd/Cython/Utility/Exceptions.c#L1007C1-L1007C52 |
So IIUC you are proposing to add a new public API to set the |
This is to fix the build for Scipy, which includes this header from C++ code which seems to really care about such things.
To be clear, I didn't confirm that this fixes the Scipy build -- there are a bunch of other patches required to its build system to even compile with Python 3.13 and I'm not sure it's worth the effort there.
But this warning seems legitimate and the fix is harmless. It's possible there are other errors in Scipy behind it, but this is the only struct initializer added in #117144.