Skip to content
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

clear_weakref() in Objects/weakrefobject.c is Confusing #95348

Open
ericsnowcurrently opened this issue Jul 27, 2022 · 2 comments
Open

clear_weakref() in Objects/weakrefobject.c is Confusing #95348

ericsnowcurrently opened this issue Jul 27, 2022 · 2 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ericsnowcurrently
Copy link
Member

(inspired by #95302 (comment))

In Objects/weakrefobject.c, the clear_weakref() function is used to:

  • pop the PyWeakReference object off its referenced object's tp_weaklist (a linked list)
  • clear (but not call) the callback, if any

However, in the places it is used, it isn't clear what it's doing. When adding a new use of clear_weakref() in one of my PRs it was particularly confusing to a reviewer what the relationship was between clear_weakref() and an object's weakref list (i.e. tp_weaklist).

While we could simply rename it to "remove_and_clear_weakref", it may make sense to do the following:

  • move the linked-list part to a separate remove_weakref() that has an explicit PyWeakReference ** parameter
  • (hence, remove the linked list parts from clear_weakref())
  • add remove_weakref_from_referent() which calls GET_WEAKREFS_LISTPTR(self->wr_object) and calls remove_weakref() on it
  • add a call to it everywhere we already call clear_weakref()

Other observations:

  • currently we aren't reseting self->hash or self->vectorcall to NULL in clear_weakref()
  • we may be leaking weakref objects PyObject_ClearWeakRefs() when the weakref doesn't have a callback
@ericsnowcurrently ericsnowcurrently added the type-feature A feature request or enhancement label Jul 27, 2022
@PurityLake
Copy link
Contributor

I think I will give this a go

@PurityLake
Copy link
Contributor

PurityLake commented Jul 29, 2022

Would there be an issue of this breaking existing code though?

EDIT:

Ok silly question. It isn't a function that is exported.

PurityLake added a commit to PurityLake/cpython that referenced this issue Aug 1, 2022
PurityLake added a commit to PurityLake/cpython that referenced this issue Aug 1, 2022
PurityLake added a commit to PurityLake/cpython that referenced this issue Aug 1, 2022
PurityLake added a commit to PurityLake/cpython that referenced this issue Aug 5, 2022
PurityLake added a commit to PurityLake/cpython that referenced this issue Aug 5, 2022
PurityLake added a commit to PurityLake/cpython that referenced this issue Aug 5, 2022
PurityLake added a commit to PurityLake/cpython that referenced this issue Aug 5, 2022
@iritkatriel iritkatriel added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants