[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[Gregory P. Smith]

Change by Gregory P. Smith :


--
nosy: +gregory.p.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[Dong-hee Na]

Change by Dong-hee Na :


--
nosy: +corona10

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +30286
pull_request: https://github.com/python/cpython/pull/32210

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 7fc39a21cb85163a456eab91b52e5fe85e7f7e3e by Victor Stinner in 
branch 'main':
bpo-47164: Add _PyCFunctionObject_CAST() macr (GH-32190)
https://github.com/python/cpython/commit/7fc39a21cb85163a456eab91b52e5fe85e7f7e3e


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:


New changeset f0bc69485677ae8973685866ada0982976d3878f by Victor Stinner in 
branch 'main':
bpo-47164: Add _PyCFunction_CAST() macro (GH-32192)
https://github.com/python/cpython/commit/f0bc69485677ae8973685866ada0982976d3878f


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:


New changeset c14d7e4b816134b8e93ece4066a86d229631ce96 by Victor Stinner in 
branch 'main':
bpo-47164: Add _PyASCIIObject_CAST() macro (GH-32191)
https://github.com/python/cpython/commit/c14d7e4b816134b8e93ece4066a86d229631ce96


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:

After reading Mark's comments, I reworked my GH-32190 PR to only use the CAST 
macros in other macros, not in the C code. The CAST macros are not used in such 
code pattern:

 else if (PyCFunction_Check(func))
-return ((PyCFunctionObject*)func)->m_ml->ml_name;
+return _PyCFunctionObject_CAST(func)->m_ml->ml_name;

In fact, this change doesn't bring any value.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:

> I've had to debug a segfault before only because the inline function 
> implicitly cast its arguments, and it was accessing a non-existent member. If 
> it were a macro it would access the struct member directly, and the compiler 
> would be able to catch that and warn me before runtime.

This is part of Python C API legacy and as I wrote, it's not going to change 
soon. The minor enhancement that we can do is to inject an assertion when 
Python is built in debug mode.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[Ken Jin]

Ken Jin  added the comment:

@Victor,

I'm not against (A) or any of the PRs you proposed. They look fine to me. My 
concern was with certain static inline functions (there are none here :). The 
_CAST macros have genuinely helped me debug code before.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:

GH-32192 "Add _PyCFunction_CAST() macro" is special. It's used to define 
functions in PyTypeObject.tp_methods or PyModuleDef.m_methods.

Casting a function pointer can cause issues with Control Flow Integrity (CFI) 
implemented in LLVM. The _thread module has an undefined behavior fixed by the 
commit 9eea6eaf23067880f4af3a130e3f67c9812e2f30 of bpo-33015. Previously, a 
cast ignored that the callback has no return value, whereas pthread_create() 
requires a function which as a void* return value.

To fix compiler warnings, the (PyCFunction)func cast was replaced with the 
(PyCFunction)(void(*)(void))func cast to fix bpo-bpo-33012 GCC 8 compiler 
warning:

warning: cast between incompatible function types (...)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:

Hum, there are two things and maybe we are not talking about the same thing.

* (A) Modifying macros defined in the Python C API (Include/ header files) 
doing cast on their arguments to use CAST macros
* (B) Modify C code doing casts to use CAST macros

I created this issue for (A). I propose to do (B), but this part is optional.

Mark, Ken: are you fine with (A)? Is your concern about (B)?

But modifying macros to remove the cast of their argument is out of the scope 
of this issue. As I wrote, it would add compiler warnings, something that I 
would like to avoid. See:
https://peps.python.org/pep-0670/#cast-pointer-arguments

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:

> By the way, the current Python C API is not fully compatible with C++. (...)

I created bpo-47165 "[C API] Test that the Python C API is compatible with C++".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

STINNER Victor  added the comment:

> I think that adding macros makes readability worse.

See the attached PRs, I can remove multiple layers of parenthesis in macros by 
using CAST macros.

> ```
> PyObject *func = ...;
> int flags = PyCFunction_GET_FLAGS(func);
> ```
>
> is dangerous.

Right.

PEP 670 proposes to remove the cast, but only in the limited C API version 3.11 
or newer. Sadly, I don't think that we can remove the cast in the general 
Python C API, it would simply add too many compiler warnings in existing C 
extensions which previously built without warnings. (See also PEP 670 
discussions on python-dev).

Currently, PyCFunction_GET_FLAGS() doesn't check its argument. The macro 
documentation is quite explicit about it:

/* Macros for direct access to these values. Type checks are *not*
   done, so use with care. */

My GH-32190 PR adds a check in debug mode. So using PyCFunction_GET_FLAGS() in 
debug mode is safer with this PR.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[Ken Jin]

Ken Jin  added the comment:

> tiny inline function obscures the meaning of code

Sadly I have to agree with Mark for that specific case. I've had to debug a 
segfault before only because the inline function implicitly cast its arguments, 
and it was accessing a non-existent member. If it were a macro it would access 
the struct member directly, and the compiler would be able to catch that and 
warn me before runtime.

However, for the case of _PyCFunctionObject_CAST, I'm not against it. The 
assert(PyCFunction_Check) pattern has genuinely helped me catch problems in the 
case of similar macros.

--
nosy: +kj

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +30270
pull_request: https://github.com/python/cpython/pull/32192

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +30269
pull_request: https://github.com/python/cpython/pull/32191

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[Mark Shannon]

Mark Shannon  added the comment:

The problem in the example you give is the need for the cast in the first 
place. If `func` were a `PyCFunctionObject *` instead of a `PyObject *`, then 
there would be no cast.


Making the casts explicit serves as a reminder that a type check is needed.

```
PyObject *func = ...;
int flags = PyCFunction_GET_FLAGS(func);
```

is dangerous.

```
PyObject *obj = ...;
if (PyCFunction_Check(obj)) {
PyCFunctionObject *func = (PyCFunctionObject *)obj;
int flags = func->m_ml->ml_flags;
```

is safe.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[Mark Shannon]

Mark Shannon  added the comment:

I think that adding macros makes readability worse.

The macro is only more readable if you already know what it does.
If you don't, then you need to look up the macro, and understand the cast in 
the macro (which is harder than understanding the original cast).


In general, I find the excessive use of macros and tiny inline function 
obscures the meaning of code, and makes it hard to reason about what the code 
is doing.

A few well chosen macros (like Py_INCREF(), etc) can definitely help 
readability, but if there are too many then anyone reading the code ends up 
having to lookup loads of macro definitions to understand the code.



Without the macro, the reader needs to parse the cast, which is admittedly a

--
nosy: +Mark.Shannon

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

Change by STINNER Victor :


--
keywords: +patch
pull_requests: +30268
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/32190

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue47164] [C API] Add private "CAST" macros to clean up casts in C code

[STINNER Victor]

New submission from STINNER Victor :

Last years, I started to add "CAST" macros like:

#define _PyObject_CAST(op) ((PyObject*)(op))
#define _PyType_CAST(op) (assert(PyType_Check(op)), (PyTypeObject*)(op))

Example of usage:

#define PyObject_TypeCheck(ob, type) PyObject_TypeCheck(_PyObject_CAST(ob), 
type)

These macros avoids parenthesis. Example without CAST macro:

#define PyCFunction_GET_FLAGS(func) \
(((PyCFunctionObject *)func) -> m_ml -> ml_flags)

Currently, inline cast requires adding parenthesis:

   ((PyCFunctionObject *)func)

IMO it's more readable with a CAST macro:

#define _PyCFunction_CAST(func) ((PyCFunctionObject *)func)
#define PyCFunction_GET_FLAGS(func) \
(_PyCFunction_CAST(func)->m_ml->ml_flags)


I propose to add more CAST macros.

By the way, the current Python C API is not fully compatible with C++. 
"(type)expr" C syntax is seen as "old-style cast" in C++ which recommends 
static_cast(expr), reinterpret_cast(expr), or another kind of cast. 
But I prefer to discuss C++ in a separated issue ;-) IMO without considering 
C++, adding CAST macros is worth it for readability.

I am preparing pull requests for add CAST macros and use existing CAST macros.

--
components: C API
messages: 416350
nosy: vstinner
priority: normal
severity: normal
status: open
title: [C API] Add private "CAST" macros to clean up casts in C code
versions: Python 3.11

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com