gh-106104: Use public weakref APIs in sqlite3 #106105
Conversation
The sqlite3 extension module should be built using the public API, so we're removing the Py_BUILD_CORE_MODULE define that was introduced as part of the pythongh-105927 changes.
| @@ -102,12 +97,18 @@ pysqlite_close_all_blobs(pysqlite_Connection *self) | |||
| { | |||
| for (int i = 0; i < PyList_GET_SIZE(self->blobs); i++) { | |||
| PyObject *weakref = PyList_GET_ITEM(self->blobs, i); | |||
| PyObject *blob = _PyWeakref_GET_REF(weakref); | |||
| if (blob == NULL) { | |||
| if (!PyWeakref_Check(weakref)) { | |||
| continue; | |||
There was a problem hiding this comment.
Is it possible that blobs contain something which is not a weakref?
| Py_DECREF(blob); | ||
| PyObject *blob; | ||
| if (PyWeakref_GetRef(weakref, &blob) < 0) { | ||
| PyErr_WriteUnraisable((PyObject *)self); |
There was a problem hiding this comment.
We should make _PyErr_WriteUnraisableMsg() public, since this one logs a message which is hard to get, it gives no context.
Not a big issue, but historically, the sqlite3 extension module has been implemented (mostly) using public API. See also: |
|
Related: I see that |
How is it an issue to use the internal C API? The internal C API is written for best performances thanks to inlining when you control the argument types. |
| PyErr_WriteUnraisable((PyObject *)self); | ||
| continue; | ||
| } | ||
| if (item) { |
There was a problem hiding this comment.
I suggest if (!item) continue; to avoid one indentation level.
Well, I'll let you reply yourself (in #88140 (comment)) 😄:
|
|
I had great hope in being able to convert some stdlib extensions to the limited C API, but then i fall into Argument Clinic and I lost faith. I mean that I found more urgent things to do. If someone is volunteer to attempt that again, I'm very supportive. Maybe we might try to define some embargo on abusing private and internal APIs in Python stdlib extensions 😁 It should be the exception, not the default. I'm not sure. |
|
Hi, what's the status of this PR? About converting some stdlib C extensions to the limited C API, you can see: #85283 |
|
I don't plan to follow this up right now. If we decide to convert |
The sqlite3 extension module should be built using the public API, so
we're removing the Py_BUILD_CORE_MODULE define that was introduced as
part of the gh-105927 changes.