Skip to content

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Dec 8, 2022

If _Py_TYPEOF() is not available, the limited C API now implements the Py_CLEAR() macro as a function call which hides implementation details. The function uses memcpy() of <string.h> which is not included by <Python.h>.

  • Add private _Py_Clear() function.
  • Add _Py_Clear() to Misc/stable_abi.toml.
@netlify
Copy link

netlify bot commented Dec 8, 2022

Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit 5dac437
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/63926d360234d3000869cffc
@vstinner
Copy link
Member Author

vstinner commented Dec 8, 2022

Oh, I forgot that Py_CLEAR() is already part of the limited C API and the limited C API no longer includes <string.h> since Python 3.11.

I already proposed a few times to provide Py_SETREF() and Py_CLEAR() as function calls. Well, here is my attempt to introduce a function implementation of the Py_CLEAR() macro :-)

If Py_SETREF() and Py_XSETREF() macros are added to the limited C API (PR #99575), they will need a similar function implementation.

I would really prefer hiding the implementation details and avoiding to include <string.h> in the limited C API <Python.h>.

cc @serhiy-storchaka @encukou

@vstinner
Copy link
Member Author

vstinner commented Dec 8, 2022

In this PR, I chose not use the function implementation if the __typeof__() function is available for best performance.

If _Py_TYPEOF() is not available, the limited C API now implements the Py_CLEAR() macro as a function call which hides implementation details. The function uses memcpy() of <string.h> which is not included by <Python.h>. * Add private _Py_Clear() function. * Add _Py_Clear() to Misc/stable_abi.toml.
@serhiy-storchaka
Copy link
Member

I suggest to not fix what was not broken. Py_CLEAR and Py_SETREF were macros, and worked pretty well as macros. Problems started when you converted them to functions.

@vstinner
Copy link
Member Author

vstinner commented Dec 9, 2022

I suggest to not fix what was not broken.

I'm not sure what you are talking about, the issue #98724 described a bug that was fixed in Python 3.12.

Py_CLEAR and Py_SETREF were macros, and worked pretty well as macros. Problems started when you converted them to functions.

Py_CLEAR() and Py_SETREF() are currently implemented as macro. What makes you think that they became functions?

This change is specific to the limited API.

@serhiy-storchaka
Copy link
Member

I do not think that it is a bug. The documentation can be improved, if it confuses somebody.

If Py_CLEAR is a macro, no need to introduce _Py_Clear. It is a prelimilary pessimization.

@vstinner
Copy link
Member Author

vstinner commented Dec 9, 2022

If Py_CLEAR is a macro, no need to introduce _Py_Clear. It is a prelimilary pessimization.

Right now, calling Py_CLEAR() in a C extension using the limited C API fails because memcpy() is not defined. That's the problem I'm trying to solve.

I do not think that it is a bug. The documentation can be improved, if it confuses somebody.

About the issue #98724 : yes, it would be possible to document that Py_CLEAR() and Py_SETREF() have bugs and there are ways to work around them.

I chose to fix the bug instead. I prefer to provide an API which is not error prone. I would like to promote Py_CLEAR() and Py_SETREF() "functions" (macros), they are preventing race conditions and so more code should use them. But I would not be comfortable to have to explain that they have bugs.

Fixing that bugs makes me me comfortable to add these functions to the limited C API.

@serhiy-storchaka
Copy link
Member

I do not think there was a bug. By fixing a non-bug you introduced a bug which you are trying to fix now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment