-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-42972: Fully implement GC protocol for functools keywrapper and partial types #26363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
8ef6e98
6d7cdaf
a15b49f
0c8df1f
f9ac4c5
fc39917
07f2182
0861788
7c2f750
6313247
1539195
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,18 +147,37 @@ partial_new(PyTypeObject *type, PyObject *args, PyObject *kw) | |
return (PyObject *)pto; | ||
} | ||
|
||
static int | ||
partial_clear(partialobject *pto) | ||
{ | ||
Py_CLEAR(pto->fn); | ||
Py_CLEAR(pto->args); | ||
Py_CLEAR(pto->kw); | ||
Py_CLEAR(pto->dict); | ||
return 0; | ||
} | ||
|
||
static int | ||
partial_traverse(partialobject *pto, visitproc visit, void *arg) | ||
{ | ||
Py_VISIT(pto->fn); | ||
Py_VISIT(pto->args); | ||
Py_VISIT(pto->kw); | ||
Py_VISIT(pto->dict); | ||
Py_VISIT(Py_TYPE(pto)); | ||
return 0; | ||
} | ||
|
||
static void | ||
partial_dealloc(partialobject *pto) | ||
{ | ||
PyTypeObject *tp = Py_TYPE(pto); | ||
/* bpo-31095: UnTrack is needed before calling any callbacks */ | ||
PyObject_GC_UnTrack(pto); | ||
if (pto->weakreflist != NULL) | ||
if (pto->weakreflist != NULL) { | ||
PyObject_ClearWeakRefs((PyObject *) pto); | ||
Py_XDECREF(pto->fn); | ||
Py_XDECREF(pto->args); | ||
Py_XDECREF(pto->kw); | ||
Py_XDECREF(pto->dict); | ||
} | ||
(void)partial_clear(pto); | ||
tp->tp_free(pto); | ||
Py_DECREF(tp); | ||
} | ||
|
@@ -307,16 +326,6 @@ partial_call(partialobject *pto, PyObject *args, PyObject *kwargs) | |
return res; | ||
} | ||
|
||
static int | ||
partial_traverse(partialobject *pto, visitproc visit, void *arg) | ||
{ | ||
Py_VISIT(pto->fn); | ||
Py_VISIT(pto->args); | ||
Py_VISIT(pto->kw); | ||
Py_VISIT(pto->dict); | ||
return 0; | ||
} | ||
|
||
PyDoc_STRVAR(partial_doc, | ||
"partial(func, *args, **keywords) - new function with partial application\n\ | ||
of the given arguments and keywords.\n"); | ||
|
@@ -469,11 +478,11 @@ static PyType_Slot partial_type_slots[] = { | |
{Py_tp_setattro, PyObject_GenericSetAttr}, | ||
{Py_tp_doc, (void *)partial_doc}, | ||
{Py_tp_traverse, partial_traverse}, | ||
{Py_tp_clear, partial_clear}, | ||
{Py_tp_methods, partial_methods}, | ||
{Py_tp_members, partial_memberlist}, | ||
{Py_tp_getset, partial_getsetlist}, | ||
{Py_tp_new, partial_new}, | ||
{Py_tp_free, PyObject_GC_Del}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to keep it and maybe only change that in 3.11 (in a separated PR)? Type inheritance is more complex than what it looks. See https://bugs.python.org/issue43770 for examples of bad surprised that I got when trying to use the default implementation of tp_getattro and tp_setattro. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! |
||
{0, 0} | ||
}; | ||
|
||
|
@@ -506,8 +515,9 @@ static void | |
keyobject_dealloc(keyobject *ko) | ||
{ | ||
PyTypeObject *tp = Py_TYPE(ko); | ||
keyobject_clear(ko); | ||
PyObject_Free(ko); | ||
PyObject_GC_UnTrack(ko); | ||
(void)keyobject_clear(ko); | ||
PyObject_GC_Del(ko); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO here it's ok to hardcode PyObject_GC_Del(), but in general I would suggest to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree :) Consistency FTW |
||
Py_DECREF(tp); | ||
} | ||
|
||
|
@@ -516,6 +526,7 @@ keyobject_traverse(keyobject *ko, visitproc visit, void *arg) | |
{ | ||
Py_VISIT(ko->cmp); | ||
Py_VISIT(ko->object); | ||
Py_VISIT(Py_TYPE(ko)); | ||
return 0; | ||
} | ||
|
||
|
@@ -546,7 +557,8 @@ static PyType_Slot keyobject_type_slots[] = { | |
static PyType_Spec keyobject_type_spec = { | ||
.name = "functools.KeyWrapper", | ||
.basicsize = sizeof(keyobject), | ||
.flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_DISALLOW_INSTANTIATION, | ||
.flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_DISALLOW_INSTANTIATION | | ||
Py_TPFLAGS_HAVE_GC), | ||
.slots = keyobject_type_slots | ||
}; | ||
|
||
|
@@ -560,14 +572,15 @@ keyobject_call(keyobject *ko, PyObject *args, PyObject *kwds) | |
if (!PyArg_ParseTupleAndKeywords(args, kwds, "O:K", kwargs, &object)) | ||
return NULL; | ||
|
||
result = PyObject_New(keyobject, Py_TYPE(ko)); | ||
result = PyObject_GC_New(keyobject, Py_TYPE(ko)); | ||
if (result == NULL) { | ||
return NULL; | ||
} | ||
Py_INCREF(ko->cmp); | ||
result->cmp = ko->cmp; | ||
Py_INCREF(object); | ||
result->object = object; | ||
PyObject_GC_Track(result); | ||
return (PyObject *)result; | ||
} | ||
|
||
|
@@ -621,12 +634,13 @@ functools_cmp_to_key(PyObject *self, PyObject *args, PyObject *kwds) | |
return NULL; | ||
|
||
state = get_functools_state(self); | ||
object = PyObject_New(keyobject, state->keyobject_type); | ||
object = PyObject_GC_New(keyobject, state->keyobject_type); | ||
if (!object) | ||
return NULL; | ||
Py_INCREF(cmp); | ||
object->cmp = cmp; | ||
object->object = NULL; | ||
PyObject_GC_Track(object); | ||
return (PyObject *)object; | ||
} | ||
|
||
|
@@ -752,9 +766,9 @@ static void | |
lru_list_elem_dealloc(lru_list_elem *link) | ||
{ | ||
PyTypeObject *tp = Py_TYPE(link); | ||
Py_XDECREF(link->key); | ||
Py_XDECREF(link->result); | ||
PyObject_Free(link); | ||
Py_CLEAR(link->key); | ||
Py_CLEAR(link->result); | ||
erlend-aasland marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tp->tp_free(link); | ||
erlend-aasland marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Py_DECREF(tp); | ||
} | ||
|
||
|
@@ -1260,7 +1274,7 @@ lru_cache_dealloc(lru_cache_object *obj) | |
PyObject_ClearWeakRefs((PyObject*)obj); | ||
} | ||
|
||
lru_cache_tp_clear(obj); | ||
(void)lru_cache_tp_clear(obj); | ||
tp->tp_free(obj); | ||
Py_DECREF(tp); | ||
} | ||
|
@@ -1332,6 +1346,7 @@ lru_cache_tp_traverse(lru_cache_object *self, visitproc visit, void *arg) | |
lru_list_elem *next = link->next; | ||
erlend-aasland marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Py_VISIT(link->key); | ||
Py_VISIT(link->result); | ||
Py_VISIT(Py_TYPE(link)); | ||
link = next; | ||
} | ||
Py_VISIT(self->func); | ||
|
@@ -1340,6 +1355,7 @@ lru_cache_tp_traverse(lru_cache_object *self, visitproc visit, void *arg) | |
Py_VISIT(self->lru_list_elem_type); | ||
Py_VISIT(self->cache_info_type); | ||
Py_VISIT(self->dict); | ||
Py_VISIT(Py_TYPE(self)); | ||
return 0; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to always start by visiting the type in all traverse functions. So same remark for other traverse functions.
I like to look into partialobject structure and visit them by their definition order. Technically, ob_type is the first one :-D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll apply to all remaining PR's :)