-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-35059: Convert Py_INCREF() to static inline function #10079
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
Misc/NEWS.d/next/C API/2018-10-24-17-29-59.bpo-35059.hjUsEZ.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Build/2018-10-25-11-07-41.bpo-35059.6QP-kK.rst
Outdated
Show resolved
Hide resolved
Would you convert |
My long term plan is to convert most macros to static inline function, but for this PR, I prefer to restrict the number of converted macros. Ok for that once since it's used directly by _Py_DECREF() ;-) |
I reverted _Py_Dealloc() change: it's more complex than what I expected. See code in object.c:
I'm not sure how to handle this case. This PR is already complex enough. I changed my mind, I don't think that _Py_Dealloc() should be changed in the same PR. I will do it in a following PR. |
Oh. I merged master into my PR and now the test fails on Windows :-( "c:\projects\cpython\include\object.h(774): error C2065: '_Py_tracemalloc_config': undeclared identifier" I wrote PR #10091 to try to fix the issue. |
Oh, I had to move _PyTraceMalloc_NewReference() inside object.h... Converting a macro into a proper function makes "dependencies" issues more obvious and requires to fix the code :-) |
Hello Victor, Your changes look to be missing the "extern" declarations. Doing that will provide the compiler the knowledge of where to put the non-inline version of the function. E.g. if you do it, you can take the address of a function that is declared inline (at least, it works for me on GCC). It is simple to do. E.g. in object.c put: See https://www.greenend.org.uk/rjk/tech/inline.html. I think we should use strategy number 3 (if it works with all the compilers we care about). |
Thinking about this some more, if we can't rely on compilers to implement the C99 behavior, I'm not sure this project of changing macros to inline functions is worth doing. Providing an implementation for compilers that can't handle it will be ugly. It would be like strategy number 4 in the link above. Too ugly IMHO and we should just keep the macros. If that does turn out to be the case, I'm sad. C99 was adopted as an ANSI standard in 2000. You would think 18 years would be enough time to implement it. |
(Currently, you cannot get the address of Py_INCREF(), since it's a macro. So nobody does that.) I'm using "static inline" but attribute((always_inline)) for GCC and clang and __force_inline for MSVC. I had to tune MSVC options to also inline in debug mode: PR #10094. If the function is not inline, it's fine, it doesn't break the ABI: https://bugs.python.org/issue35059#msg328426 IMHO the main drawback is the function is not inlined is that the machine code of the function can be duplicated if I understood properly. In my experience, Py_INCREF() is now always compiled, in release and debug mode: https://bugs.python.org/issue35059#msg328421 Moreover, I chose "static inline" because it is now required by the PEP 7 :-) https://www.python.org/dev/peps/pep-0007/#c-dialect And it's already used in pydtrace.h (I already replaced "static inline" with my new Py_STATIC_INLINE() macro.)
Python 3.6 and newer requires a C compiler which supports "static inline" (PEP 7, again). |
Sure but I was thinking of the case of making some other functions inline, e.g.
I am far from a C compiler expert but it is my understanding that this is the main downside of the "static inline" style (GCC style?). If you use just "inline" and then provide the "extern inline" declaration in the .c unit, you get a single non-inlined version.
If that's the best way, fine. I wonder though if we should change PEP 7 to recommend the C99 style ("inline" in header, "extern inline" in .c unit). I like it better because you avoid the multiple copies problem. A downside to the C99 style is that the compiler is free to not inline the function if it decides it is too large (or whatever). That could be a pro or con I guess, depending on what you are trying to do. |
* Convert Py_INCREF() and Py_DECREF() macros into static inline functions. * Remove _Py_CHECK_REFCNT() macro: code moved into Py_DECREF().
I rewrote my PR to no longer use Py_STATIC_INLINE(), but use directly "static inline", as requested by @benjaminp: see https://bugs.python.org/issue35059#msg328746 |
_Py_ForgetReference() macros to static inline functions.
_Py_NegativeRefcount() reports the correct filename.
https://bugs.python.org/issue35059