-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-39573: Porting to Python 3.10: Py_SET_SIZE() macro #20610
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
Conversation
In What's New in Python 3.10, propose Py_SET_SIZE(), Py_SET_REFCNT() and Py_SET_TYPE() macros for backward compatibility with Python 3.9 and older.
@tacaswell @njsmith @corona10: Would you mind to review this documentation change? |
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, thanks!
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
Doc/whatsnew/3.10.rst
Outdated
compatibility, this macro can be used:: | ||
|
||
#if PY_VERSION_HEX < 0x030900A4 | ||
# define Py_SET_REFCNT(obj, refcnt) do { Py_REFCNT(obj) = (refcnt); } while (0) |
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.
@tacaswell: In one of your PR, you wrote "typ" instead of "type". Was it a deliberate typo? Is there an issue in C++? Should I rename the variable to "new_type"?
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.
Oops, my question is on the second parameter of Py_SET_TYPE() :-)
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.
I built Py_SET_TYPE() macro with g++ and Python 3.8: I don't have any warning or error. "type" sounds like a valid parameter type.
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.
I am not sure, that code come from @eric-wieser via numpy/numpy#16417 (comment)
Doc/whatsnew/3.10.rst
Outdated
#if PY_VERSION_HEX < 0x030900A4 | ||
# define Py_SET_TYPE(obj, type) do { Py_TYPE(obj) = (type); } while (0) | ||
#endif |
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 doesn't behave the same way as the function in 3.9. I think a more accurate macro would be:
#if PY_VERSION_HEX < 0x030900A4 | |
# define Py_SET_TYPE(obj, type) do { Py_TYPE(obj) = (type); } while (0) | |
#endif | |
#if PY_VERSION_HEX < 0x030900A4 | |
# define Py_SET_TYPE(obj, type) ((Py_TYPE(obj)) = (type), (void)0) | |
#endif |
which can be used as part of an expression like the function version of Py_SET_TYPE
can:
return Py_SET_TYPE(my_obj, some_type), my_obj;
is legal in 3.9 and with my macro, but illegal with this PR.
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.
Oh wow, the C language is so surprising sometimes :-) I designed Py_SET_SIZE() as a static inline function to ensure that the result is void and to prevent strange usage. But I didn't know that return Py_SET_TYPE(my_obj, some_type), (...);
was legit.
I updated macros to use , (void)0
. According to https://stackoverflow.com/a/25021889 this syntax is endorsed by the C standard, since NDEBUG uses #define assert(ignore) ((void)0)
.
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.
Thanks for finding some precedent. I've seen "always use parens and do { } while(0);
" given as macro advice forever, but I don't remember ever seeing this case before.
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.
In the meanwhile, I created the backport compatiblity header file :-) It defines a static inline function which returns void:
https://github.com/pythoncapi/pythoncapi_compat/blob/219cdd858595fb5d478117b0ef4758fedc3d2365/pythoncapi_compat.h#L29
Maybe tomorrow when it will be more mature (than 1 hour old), numpy might want to use it.
@eric-wieser @tacaswell: Would you mind to review the updated PR? |
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 reasonable to me. Maybe we should try committing this modified macro to numpy first just to prove it works.
@eric-wieser @tacaswell: Do you want to try that first? |
Just did, CI should be running now |
I see numpy/numpy#16501 :-) |
Numpy looks happy with this spelling. |
Thanks for the reviews! |
In What's New in Python 3.10, propose Py_SET_SIZE(), Py_SET_REFCNT() and Py_SET_TYPE() macros for backward compatibility with Python 3.9 and older.
In What's New in Python 3.10, propose Py_SET_SIZE(), Py_SET_REFCNT()
and Py_SET_TYPE() macros for backward compatibility with Python 3.9
and older.
https://bugs.python.org/issue39573