There is a lot of macros like:

    #define PyObject_TypeCheck(ob, tp) \
    (Py_IS_TYPE(ob, tp) || PyType_IsSubtype(Py_TYPE(ob), (tp)))

These work fine until an argument happen to contain a comma. That’s possible as a result of other macro’s expansion. E.g. if U(x) is defined as x,

    PyObject_TypeCheck(ob, U(f<a,b>(c)))

expands to

    (Py_IS_TYPE(ob, f<a,b>(c)) || ...)

but < and > aren’t treated as brackets by the preprocessor so Py_IS_TYPE is now invoked with 3 arguments instead of just 2, breaking module compilation.

As arguments are expected to be values, surrounding each with parentheses solves the problem. But there are many such macros so that’s not an one-line fix.

Note: the example is from PyGLM (simplified), it doesn’t compile on 3.9 due to this issue.

This feels like it's more of an issue with the C++ compiler you're using. (I can tell it's C++ because of template syntax)

This bug is valid, that macro and likely others aren't up to best practices.

C macros are a PITA to get right.

I'm replacing macros with static inline functions to avoid issues like this one (ex: bpo-35059). Are you interested to convert the PyObject_TypeCheck() macro to a static inline function?

"""
There is a lot of macros like:
    #define PyObject_TypeCheck(ob, tp) \
    (Py_IS_TYPE(ob, tp) || PyType_IsSubtype(Py_TYPE(ob), (tp)))
These work fine until an argument happen to contain a comma.
"""

This macro also has a common bug of macros: if one argument has a side effect, it is executed twice! For example, if an argument is a function call, the function is called twice.

See:

• https://gcc.gnu.org/onlinedocs/cpp/Macro-Pitfalls.html
• https://vstinner.github.io/split-include-directory-python38.html "Convert macros to static inline functions"

Are you interested to convert the PyObject_TypeCheck() macro to a static inline function?

I'd like to give it a shot if it's ok for you all.

Silence implies consent? I'll throw up a PR, then :)

I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

That sounds reasonable.

New changeset 4bb2a1e by Erlend Egeberg Aasland in branch 'master':
bpo-43181: Convert PyObject_TypeCheck to static inline function (GH-24533)
4bb2a1e

Thanks Vitaliy for the bug report and Erlend for the fix ;-)

For Python 3.9 and older, a workaround is to wrap the call to PyObject_TypeCheck() with your own static inline function.

Thanks for the fix, it works for my use case. (btw that was
    #define U(...) __VA_ARGS__
and not what I wrote).

I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

For Python 3.9 and older, a workaround is to wrap the call to PyObject_TypeCheck() with your own static inline function.

For Python 3.8 that fix wouldn’t be needed as the tp argument was parenthesised in the macro.

Yet... the first argument is still unshielded, passed to a macro that expects one single macro argument. That’s not a regression, it wasn’t shielded in 3.8 either, but why not just parenthesise each macro argument that denotes an expression (as opposed to e.g. name)?

Yet... the first argument is still unshielded, passed to a macro that expects one single macro argument.

Are you talking about the current code in the master branch or the 3.9 branch? If you are talking about the _PyObject_CAST(ob) call, would you mind to elaborate how it is an issue? The code in master LGTM.

That’s not a regression, it wasn’t shielded in 3.8 either, but why not just parenthesise each macro argument that denotes an expression (as opposed to e.g. name)?

In the master branch, Include/ and its subdirectories contains 158 .h files for a total of 20K lines of C code. It's not easy to check all files. If you spotted bugs, would you mind to give examples, or a way to automatically detect bugs?

As I wrote previously, I dislike macros. If someone is changed, I would prefer to convert the function into a static inline function which doesn't have macros pitfalls.

As I wrote previously, I dislike macros. If someone is changed, I would prefer to convert the function into a static inline function which doesn't have macros pitfalls.

Should we create a meta issue for this? Most macros are trivial, and can safely be converted, given that they're not used as l-values. Converting one header file at the time would make it easy to review, and it would be possible to make good progress in a few months time.

If possible, I would prefer to only replace macros with static inline functions if the changes avoids clear macro pitfalls. It's not because macros have pitfalls that we must replace them all blindly.

Also, this issue is closed. At this point, I'm not convinced that it's worth it to fix PyObject_TypeCheck() in Python 3.8 and 3.9, so I leave the issue closed.

If possible, I would prefer to only replace macros with static inline functions if the changes avoids clear macro pitfalls.

Yes, of course. Should I create a meta issue for this, or do you prefer raising one issue pr. fix?

You can create a single issue, and later we will see if it's needed to split it into sub-issues.

Me:

I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

Sometimes. I'm wise :-D This change broken 2 extension modules which rely on the fact that the macro magically add parenthesis:

python-cvxopt: "if Matrix_Check((PyObject *)val)" with: "#define Matrix_Check(self) PyObject_TypeCheck(self, &matrix_tp)"
https://bugzilla.redhat.com/show_bug.cgi?id=1941557
PR proposed: cvxopt/cvxopt#190

python-Bottleneck: "if PyArray_Check(a_obj) {" with: "#define PyArray_Check(op) PyObject_TypeCheck(op, &PyArray_Type)"
https://bugzilla.redhat.com/show_bug.cgi?id=1941559

"if func(...) {" is not valid C code, but the old macro added magically parenthesis!

It's hard to say if the Python change is a backward incompatible or not... I prefer to keep it and fix the few broken C extensions.