On 08. 02. 22 17:27, Victor Stinner wrote:
Hi Petr,
Thanks for the SC review, it's very helpful! I know that it's a big PEP :-)
On Tue, Feb 8, 2022 at 11:33 AM Petr Viktorin <pvikt...@redhat.com> wrote:
*All other things being equal, static inline functions are better than
macros.*
Specifically, inline static functions should be preferred over
function-like macros in new code. Please add a note about this to PEP 7.
Ok, I created: https://github.com/python/peps/pull/2315
*When there is no backwards compatibility issue, macros can be changed
to static inline functions.*
In effect, the SC accepts the first part of the PEP, except cases where:
a macro is converted to macro and function ("Cast to PyObject*"),
Right now, a large number of macros cast their argument to a type. A
few examples:
#define PyObject_TypeCheck(ob, type)
PyObject_TypeCheck(_PyObject_CAST(ob), type)
#define PyTuple_GET_ITEM(op, i) (_PyTuple_CAST(op)->ob_item[i])
#define PyDict_GET_SIZE(mp) (assert(PyDict_Check(mp)),((PyDictObject
*)mp)->ma_used)
When I look at the Rationale points, and for the first three of these
macros I didn't find any that sound very convincing:
- Functions don't have macro pitfalls, but these simple macros don't
fall into the pits.
- Fully defining the argument types means getting rid of the cast,
breaking some code that uses the macro
- Debugger support really isn't that useful for these simple macros
- There are no new variables
- Macro tricks (parentheses and comma-separated expressions) are needed,
but they're present and tested.
Sure, if these were written now, they should be static inline functions.
But I don't think it's worth a massive change to working code.
The "massive change to working code" part is important. Such efforts
tend to have unexpected issues, which have an unfortunate tendency to
surface before release and contribute to release manager burnout.
#define PyWeakref_GET_OBJECT(ref) (... ((PyWeakReference
*)(ref))->wr_object ...)
That one looks more reasonable. How common is it?
How should we identify which macros are dangerous enough to need
changing now?
Does it mean that these macros must remain macros?
For now, yes.
I hope that changing the macros that are OK will make it easier to
review the remaining ones, and it might also help future versions of the
PEP to focus on the problematic cases.
Maybe it might also make sense to go from the other side: are there any
macros that are clearly dangerous and should be changed ASAP?
(I hope not -- those are hopefully handled one-by-one in individual issues.)
PEP 670 proposes adding a macro to still cast their arguments to the
requested type so the PEP doesn't add any new compiler warnings. The
PEP proposes to consider removing these implicit casts later (to
reduce the scope and limit annoyance caused by the PEP).
If the macro is converted to a static inline function without such
convenient macro, C extensions which don't pass the expected types
will get spammed by warnings. Well, it's easy to fix, and once it is
fixed, the code remains compatible with old Python versions.
I'm not sure that I understood the SC statement here: does it mean
that these specific macros (which cast their arguments) must remain
macros, or that it's ok to convert them to static inline functions
(even if it can emit new warnings)?
You understood correctly. It's not clear that there is enough reason to
mass-change working code.
If the SC is ok to add new compiler warnings, I'm fine with it. Most
functions are documented with an exact type, they are not documented
to cast implicitly their arguments to the expected types.
For example, Py_INCREF() expects a PyObject* type in the documentation:
https://docs.python.org/dev/c-api/refcounting.html#c.Py_INCREF
--
Or is the problem that having a macro to wrap a static inline function
requires to change the function name? Well, technically, it's not
needed... I wrote a PR so the macro and the function have the same
name:
https://github.com/python/cpython/pull/31217
For example, the Py_INCREF() macro now calls Py_INCREF(), instead of
calling _Py_INCREF().
Static inline functions are not part of the ABI since they are
inlined, but it's maybe better to avoid the "_" prefix to avoid
confusion in debuggers and profilers (especially when comparing old
and new Python versions).
and where the return value is removed.
In this case, I propose to leave these macros unchanged in PEP 670 in
this case, as it already excludes macros which can be used as l-values
(macros changed by PEP 674).
I would prefer to have the whole PEP 670 either accepted or rejected.
I'm not comfortable with a half-accepted status, it's misleading for
readers.
The SC is *not sure yet* if it can be accepted as-is.
It continues to be like any other draft PEP: a proposal, which can still
be changed.
You now can:
- wait for the SC to discuss the rest of the proposal (which will take
time, and will probably involve more public discussion, like this).
- withdraw the PEP and write a new one.
- rewrite the PEP to focus on the remaining issues.
Petr
(*not* writing on behalf of the entire SC this time)
_______________________________________________
Python-Dev mailing list -- python-dev@python.org
To unsubscribe send an email to python-dev-le...@python.org
https://mail.python.org/mailman3/lists/python-dev.python.org/
Message archived at
https://mail.python.org/archives/list/python-dev@python.org/message/Y7XPHKPHXDEL2PTL4SR57DD55PPBNXQH/
Code of Conduct: http://python.org/psf/codeofconduct/
