port extension modules' macros of `get_module_state()` to inline function.

[shihai1991]

BPO	39968
Nosy	@tim-one, @rhettinger, @vstinner, @encukou, @corona10, @shihai1991
PRs	bpo-39968: move extension modules' macros of get_module_state() to inline function. #19017 bpo-39968: Fix a typo error of readline #19028

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-03-16.18:24:56.293>
created_at = <Date 2020-03-15.13:42:06.276>
labels = ['extension-modules', 'type-feature', '3.9']
title = "port extension modules' macros of `get_module_state()` to inline function."
updated_at = <Date 2020-03-16.18:24:56.292>
user = 'https://github.com/shihai1991'

bugs.python.org fields:

activity = <Date 2020-03-16.18:24:56.292>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-03-16.18:24:56.293>
closer = 'vstinner'
components = ['Extension Modules']
creation = <Date 2020-03-15.13:42:06.276>
creator = 'shihai1991'
dependencies = []
files = []
hgrepos = []
issue_num = 39968
keywords = ['patch']
message_count = 15.0
messages = ['364231', '364278', '364315', '364316', '364317', '364320', '364321', '364327', '364329', '364332', '364333', '364339', '364343', '364347', '364349']
nosy_count = 6.0
nosy_names = ['tim.peters', 'rhettinger', 'vstinner', 'petr.viktorin', 'corona10', 'shihai1991']
pr_nums = ['19017', '19028']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue39968'
versions = ['Python 3.9']

[shihai1991]

as victor and petr said in PR18613：inline function is more better than macros, so I plan move all extension modules' macros of get_xx_state() to inline function.

Note: some inline get_xx_state() can not be used directly in tp_traverse、tp_free and tp_clear(bpo-39824).

[rhettinger]

Tim, do you approve of this kind of wholesale macro-to-inline function conversion?

My thought is that in most cases, there is no advantage to conversion and that sometimes there will be minor disadvantage in code generation when the macro is used across files. I like inline functions as well as next person, but that doesn't mean all macros have to be rewritten. Mostly, this seems like unnecessary code churn.

[vstinner]

New changeset f707d94 by Hai Shi in branch 'master':
bpo-39968: Convert extension modules' macros of get_module_state() to inline functions (GH-19017)
f707d94

[vstinner]

There are multiple good reasons to replace macros with static inline functions:

• Parameter types and return value are well defined
• Less error-prone: avoid completely https://gcc.gnu.org/onlinedocs/gcc-9.3.0/cpp/Macro-Pitfalls.html#Macro-Pitfalls
• Variables have a well defined scope, reduce the risk of unexpected side effects

I merged the PR 19017: it uses better names for the function and it adds assert(state != NULL).

Be careful of bpo-39824: in some cases, the module state *can* be NULL: but I checked, and it's not the case here. Moreover, bpo-39824 may prevent NULL module state (let's see ;-)).

Thanks Hai Shi!

minor disadvantage in code generation

Micro-benchmarks and machine code analysis was done in previous issues replacing macros with static inline functions, and the outcome is that there is no effect on performance. See for example bpo-35059: performance critical Py_INCREF() and Py_DECREF() functions are now static inline functions.

If someone is curious about the the machine code, you should use -O3 with PGO+LTO, which is what should be used on Linux distributions. If static inline functions are not inlined for whatever reason, I suggest to tune the compiler rather than moving back to ugly macros.

If you use configure --enable-shared, I now also strongly suggest to use -fno-semantic-interposition: it's now used on Fedora and RHEL to build Python libpython.

Note: Support for "static inline" is now explicitly required by the PEP-7 to build Python, since Python 3.6. And it's more and more used in Python header files.