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.

Sign up for GitHub

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

Jump to bottom

GH-92678: Expose managed dict clear and visit functions #95246

Merged
merged 4 commits into from
Jul 25, 2022
Merged

GH-92678: Expose managed dict clear and visit functions #95246

merged 4 commits into from
Jul 25, 2022

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Jul 25, 2022 •
edited by bedevere-bot
Loading

Adds the unstable API functions

PyAPI_FUNC(int) _PyObject_VisitManagedDict(PyObject *self, visitproc visit, void *arg);
PyAPI_FUNC(void) _PyObject_ClearManagedDict(PyObject *self);

I've only added a test for _PyObject_ClearManagedDict as there doesn't seem to be a straightforward way to test the visit function.

@markshannon markshannon requested a review from methane as a code owner July 25, 2022 14:52
@Fidget-Spinner Fidget-Spinner requested a review from pablogsal July 25, 2022 15:18
@pablogsal
Copy link
Member

pablogsal commented Jul 25, 2022 •
edited
Loading

I'm going with this solution instead of https://github.com/python/cpython/pull/95242/files because I really don't want to modify typeobject.c in the last beta that's only one week :S

As these functions are marked as unstable API functions, we have some space to proceed if we need to adapt things around, but for the time being is good enough for now.

If this proves insufficient, or the breakage is to big, we can always try the automatic way in typeobject.c but changes will always be needed to port to Python 3.11.

@markshannon @Fidget-Spinner Please, after I merge this, make a PR adding this change to the what's new for 3.11 on "how to port to Python 3.11" (and mark the issue as a blocker so we don't forget to merge it).

pablogsal
pablogsal reviewed Jul 25, 2022
View reviewed changes
Include/cpython/dictobject.h
@@ -83,3 +83,6 @@ typedef struct {

PyAPI_FUNC(PyObject *) _PyDictView_New(PyObject *, PyTypeObject *);
PyAPI_FUNC(PyObject *) _PyDictView_Intersect(PyObject* self, PyObject *other);

PyAPI_FUNC(int) _PyObject_VisitManagedDict(PyObject *self, visitproc visit, void *arg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is quite a bit weird: these functions receive an object and then visit/clear the dict. This is not an API for dictionary objects, it's an API for instances so I think it should not be here but in object.h or some other header.

Looking at these functions, at first sight, it seems that "self" is the dictionary and not the oject.

@pablogsal
Copy link
Member

I'm merging the PR to get this into beta 5. We can address moving the functions to a different header and the docs later.

@pablogsal pablogsal merged commit 27055d7 into python:main Jul 25, 2022
@miss-islington
Copy link
Contributor

Thanks @markshannon for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @markshannon and @pablogsal, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 27055d766ab0ee5ddcfbd1fb51fb47419af1ddba 3.11

pablogsal pushed a commit to pablogsal/cpython that referenced this pull request Jul 25, 2022
@markshannon @pablogsal
[3.11] pythonGH-92678: Expose managed dict clear and visit functions (p…
2424e69 
…ythonGH-95246).

(cherry picked from commit 27055d7)

Co-authored-by: Mark Shannon <[email protected]>
@pablogsal pablogsal mentioned this pull request Jul 25, 2022
Closed
pablogsal added a commit that referenced this pull request Jul 25, 2022
@pablogsal @markshannon
[3.11] GH-92678: Expose managed dict clear and visit functions (GH-95246
7f73194 
). (#95256)

Co-authored-by: Mark Shannon <[email protected]>
@markshannon markshannon mentioned this pull request Aug 2, 2022
Closed
markshannon added a commit that referenced this pull request Aug 4, 2022
@markshannon
Revert "[3.11] GH-92678: Expose managed dict clear and visit functions (
5dc94e7 
GH-95246). (#95256)"

This reverts commit 7f73194.
@bedevere-bot
Copy link

GH-95647 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 4, 2022
pablogsal pushed a commit that referenced this pull request Aug 4, 2022
@markshannon
Revert "[3.11] GH-92678: Expose managed dict clear and visit functions (
312dab2 
GH-95246). (#95256)" (#95647)

This reverts commit 7f73194.
@markshannon markshannon deleted the expose-managed-dict-clear-and-visit-functions branch September 26, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments

@pablogsal pablogsal pablogsal left review comments

@methane methane Awaiting requested review from methane methane is a code owner

Assignees

@pablogsal pablogsal

Labels
awaiting core review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@markshannon @pablogsal @miss-islington @bedevere-bot @Fidget-Spinner