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
Remove unused and undocumented PyGen_NeedsFinalizing() function #59293
Comments
Currently undocumented: http://docs.python.org/py3k/c-api/gen.html |
Please correct me if I'm wrong, but I think PyGen_NeedsFinalizing should not be an API function because it is only used, nor _PyGen_FetchStopIterationValue. In the attached patch I've removed PyGen_NeedsFinalizing and _PyGen_FetchStopIterationValue fom the public API. |
For the record, PyGen_NeedsFinalizing still exists but it isn't used anymore in the code base (following PEP-442). |
What should be done for the PyGen_NeedsFinalizing API? Does it need to be documented or should it be removed since it's no longer used in the code base? Thanks! |
Cheryl: it may be useful to do a code search to find out whether any third-party projects are relying on PyGen_NeedsFinalizing. |
Thanks Antoine. Do you have a good way for searching third party projects? I searched on Github and I'm getting a lot of references to the CPython filenames. I don't know how to check if anyone is making calls to the function. But, maybe that's the point and it can't be removed if there's a chance that anyone uses it? Thanks! This is what I tried on Github: |
I think https://searchcode.com/ may have a larger indexing base than GitHub alone. And, yes, you'll see many duplicates of the CPython source code... Visual inspection may be needed :-/ |
Thanks again, Antoine. I'll see what I can come up with. :-) |
My searches show references to this function in CPython cloned repositories. From @pitrous's views, I think it is better to deprecate it with a removal notice for a later release. I have deprecated the Function instead and will be removed in the next release in this PR #15702 . |
PyGen_NeedsFinalizing() was added by: commit 49fd7fa
It was used in this gcmodule.c function: /* Return true if object has a finalization method.
* CAUTION: An instance of an old-style class has to be checked for a
*__del__ method, and earlier versions of this used to call PyObject_HasAttr,
* which in turn could call the class's __getattr__ hook (if any). That
* could invoke arbitrary Python code, mutating the object graph in arbitrary
* ways, and that was the source of some excruciatingly subtle bugs.
*/
static int
has_finalizer(PyObject *op)
{
if (PyInstance_Check(op)) {
assert(delstr != NULL);
return _PyInstance_Lookup(op, delstr) != NULL;
}
else if (PyType_HasFeature(op->ob_type, Py_TPFLAGS_HEAPTYPE))
return op->ob_type->tp_del != NULL;
else if (PyGen_CheckExact(op))
return PyGen_NeedsFinalizing((PyGenObject *)op);
else
return 0;
} (2) The PEP-442 implementation made PyGen_NeedsFinalizing() useless: commit 796564c
Replaced: /* Return true if object has a finalization method. */
static int
has_finalizer(PyObject *op)
{
if (PyGen_CheckExact(op))
return PyGen_NeedsFinalizing((PyGenObject *)op);
else
return op->ob_type->tp_del != NULL;
} with: /* Return true if object has a pre-PEP 442 finalization method. */
static int
has_legacy_finalizer(PyObject *op)
{
return op->ob_type->tp_del != NULL;
} -- The last reference to PyGen_NeedsFinalizing() can be found in ceval.c:
|
The function is not documented nor tested. I searched for usage of PyGen_NeedsFinalizing() in C code in GitHub: I only found copies of the CPython code source: PyGen_NeedsFinalizing() definition in genobject.h. IHMO we can safely remove the function right now. If someone complains, we can reintroduce it later. We just have to document clearly its removal at: |
I merged Joannah's change: thanks. If anyone uses the removed function, please complain before Python 3.9.0 release :-) |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: