According to PEP3123, all accesses to ob_refcnt and ob_type MUST cast the object pointer to PyObject* (unless the pointer is already known to have that type), and SHOULD use the respective accessor macros.

I find that there are still some macros in Python use (obj)->ob_type. Though right now they may not impose any error, but as macros, they may be used with arguments not of type PyObject* later and introduce errors. So I think change them to use Py_TYPE is not a bad idea.

In Python 2.6 PyObject_HEAD was defined as

#define PyObject_HEAD                   \
    _PyObject_HEAD_EXTRA                \
    Py_ssize_t ob_refcnt;               \
    struct _typeobject *ob_type;

Every extension type structure contained fields ob_refcnt and ob_type. For the pointer of type (FooObject *) you used foo->ob_refcnt and foo->ob_type. You could also use ob = (PyObject *)foo; ob->ob_type, but this is undefined behavior, as estimated in PEP3123.

Now PyObject_HEAD is defined as

#define PyObject_HEAD                   PyObject ob_base;

Every extension type structure no longer contain fields ob_refcnt and ob_type, and you can't access them directly. For convenience and compatibility with 2.6 there were added macros Py_TYPE() and Py_REFCNT(). Direct access to ob_refcnt and ob_type is legal, just your can't use it with arbitrary pointer type without casting it to PyObject*. That just can't be compiled. If you have a pointer of type PyObject*, you can use direct access to ob_refcnt and ob_type.

Direct access to ob_type is used about 300 times in the source code. Changing all this case to use the Py_TYPE() macro causes code churn.

There is different situation with ob_refcnt. It is less used, and changing the code to use Py_REFCNT() causes less code churn. But this can make the code looks more pretty. Here is a patch (written a day ago) that makes the code to use Py_REFCNT(). I was not sure about the code that directly modifies the reference count and left it unchanged. I'm not sure that it is worth to apply this patch, may be it still makes too much code churn.

No, actually I don't mean to change all the (obj)->ob_type in Python repo. I know there are more (obj)->ob_type in it, but they are compliant with PEP3123 since obj is of type PyObject*. You don't have the need to change them. What I propose is only to change the macros that not use Py_TYPE since the macros may be used later with arguments not of type PyObject*.

I also inspect Py_REFCNT and Py_SIZE. I don't think there is need to replace (obj)->ob_refcnt since they are legal. 

So my intention is only several macros need to be altered and won't make too much code churn.

Ah, sorry. This makes sense.

But there is one catch. If allow a macro to be used with an argument not of type PyObject*, this will make harder to change the implementation and convert the macro to a function.

And this may increase the probability to make an error with calling macros with wrong argument.

I have no strong opinion about this patch.

Good point. I don't think about that.

change_some_macros_using_Py_TYPE.patch LGTM.

> Direct access to ob_type is used about 300 times in the source code. Changing all this case to use the Py_TYPE() macro causes code churn.

IHMO change_some_macros_using_Py_TYPE.patch makes the code more readable since it's harder and easier to read.

Note: Such refactoring patch must only be applied to the development branch (default).

py_refcnt.diff also LGTM, go ahead.

Most of Xiang’s changes are to Py<class>_Check() macros. I would expect them to be called with a generic PyObject pointer, and they do seem to be documented as accepting a PyObject pointer. Py_TYPE() is a macro that uses an unconditional cast. In general, these kinds of macros can hide errors that the compiler may otherwise pick up. E.g. if you accidentally pass an integer, or pointer to a pointer, etc, to Py_TYPE(), I think you will only get a warning or run-time crash, rather than a compile-time error. So while I am not that experienced with the C API, I suspect the change could have negative consequences.

My intention of the patch is that it helps reduce the possible errors described in PEP3123. I agree with Serhiy and Martin's opinion but I think the error passing a weird type argument to the macro is easier to find and debug than the undefined behaviour.

It is possible to implement type checking in C macros.

Example found on the Internet:

/* inline type checking - can mix in with other macros more easily using the comma operator,
 * C11 gives best results here */
#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L)
#  define CHECK_TYPE_INLINE(val, type) \
    (void)((void)(((type)0) != (0 ? (val) : ((type)0))), \
           _Generic((val), type: 0, const type: 0))
#else
#  define CHECK_TYPE_INLINE(val, type) \
    ((void)(((type)0) != (0 ? (val) : ((type)0))))
#endif

http://stackoverflow.com/questions/11449632/checking-types-of-macro-parameters-at-compile-time

See also Include/pymacro.h which contains some macros implementing checks tested at the compilation.

How this can help reduce the possible errors? I afraid that it rather can increase the possibility of error.

There is no undefined behaviour in current code. It was in old code (in the casting a pointer to PyObject*), but this was fixed with new PyObject_HEAD in 3.0+. 2.7 still has this undefined behaviour.

The current code is OK. But if the macro are used with arguments not of PyObject* in the future, the bugs described in PEP3123 can possibly be introduced.

The new PyObject_HEAD alone can not fix the problem. It just makes the exception happen:

Standard C has one specific exception to its aliasing rules precisely designed to support the case of Python: a value of a struct type may also be accessed through a pointer to the first field. E.g. if a struct starts with an int , the struct * may also be cast to an int * , allowing to write int values into the first field. 

You still have to make sure accessing ob_type is via a PyObject* pointer. So the PEP also says:

However, all accesses to ob_refcnt and ob_type MUST cast the object pointer to PyObject* (unless the pointer is already known to have that type), and SHOULD use the respective accessor macros.

Seems you misunderstood PEP3123. The bugs described in the PEP are gone and can't be reappear. The new PyObject_HEAD fixes the problem, it makes casting to PyObject* be defined behaviour. The accessor macros are needed only to write the code compatible with 3.x and 2.x. Without Py_TYPE() you would need to use `foo->ob_base->ob_type` or `foo->ob_type` in 3.x depending on the type of foo, and always use `foo->ob_type` in 2.x.

Noooooooooooooo!!!!!! What I am afraid most seems to happen, a lot of noice due to misunderstanding! If that is true I am quite sorry. :-( But before I admit I have to figure it out myself.

With a careful rereading of PEP3123, I think you are right. Actually the title of the PEP tells what you mean. I do misunderstand. I am quite sorry and feel shamed of making noice here and wasting time of the participants. I will think more seriously from now on. :-(

No problems! You are welcome.

I never read PEP3123 and it was a good opportunity to read this part of Python history.