◐ Shell
reader mode source ↗
Skip to content

gh-111262: Add PyDict_Pop() function [without default value nor KeyError]#112028

Merged
vstinner merged 9 commits into
python:mainfrom
vstinner:dict_pop2
Nov 14, 2023
Merged

gh-111262: Add PyDict_Pop() function [without default value nor KeyError]#112028
vstinner merged 9 commits into
python:mainfrom
vstinner:dict_pop2

Conversation

@vstinner

@vstinner vstinner commented Nov 13, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

_PyDict_Pop_KnownHash(): remove the default value and the return type becomes an int.

📚 Documentation preview 📚: https://cpython-previews--112028.org.readthedocs.build/

@bedevere-app bedevere-app Bot mentioned this pull request Nov 13, 2023
Closed
@vstinner vstinner mentioned this pull request Nov 13, 2023
Closed
@vstinner

vstinner commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

This PR is based on PR gh-111939 (which was based on PR gh-111263). This API is different:

  • Remove default_value parameter
  • result argument can be NULL: it's common that the removed value is not used in the caller, see the PR
  • Don't raise KeyError if the key is not present

@vstinner vstinner changed the title gh-111262: Add PyDict_Pop() function Nov 13, 2023
@vstinner

vstinner commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

Simplified example where PyDict_Pop() is used to remove a dict key without caring of the removed value:

static int
sys_set_object(PyInterpreterState *interp, PyObject *key, PyObject *v)
{
    PyObject *sd = interp->sysdict;
    if (v == NULL) {
        if (PyDict_Pop(sd, key, NULL) < 0) {
            return -1;
        }
        // no need to care about KeyError or Py_DECREF()
        return 0;
    }
    else {
        return PyDict_SetItem(sd, key, v);
    }
}

Example which uses the removed value:

PyObject *ob;
if (PyDict_Pop(kwargs, key, &ob) < 0) {
    goto error;
}
if (ob != NULL) {
    result->ob_item[i] = ob;
}
else {
    // no need to care about KeyError or Py_DECREF()
    result->ob_item[i] = Py_NewRef(self->ob_item[i]);
}

@vstinner

vstinner commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

The function is not added to the limited C API: @encukou asks to wait until Python will have a C API Working Group.

serhiy-storchaka
serhiy-storchaka reviewed Nov 13, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

This PR includes yet one change. It allows to pass NULL as a result address. It complicates the implementation, but makes the common use case simpler.

@vstinner

vstinner commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

As requested, I added PyDict_PopString() function.

I added tests on PyDict_PopString() and PyDict_Pop(dict, key, NULL).

I also addresssed @serhiy-storchaka's review.

@vstinner vstinner mentioned this pull request Nov 13, 2023
Open
@vstinner

vstinner commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

@serhiy-storchaka: I added more tests.

serhiy-storchaka
serhiy-storchaka approved these changes Nov 14, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM.

What interface do you prefer, Victor? This or #111939? I am ready to make a review of #111939 if you add corresponding *String variant and tests.

scoder
scoder approved these changes Nov 14, 2023
View reviewed changes

@scoder scoder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

LGTM overall, just wording fixes.

@vstinner

vstinner commented Nov 14, 2023

Copy link
Copy Markdown
Member Author

@serhiy-storchaka:

What interface do you prefer, Victor?

At the beginning, I had no idea. It was very blurry. What helped me to make my own opinion was to actually use the different proposed API and look at the updated code. I really like this API without the default value: I'm convinced by these examples.

In short, I prefer this PR.

vstinner and others added 5 commits November 14, 2023 13:19
@vstinner @scoder @pitrou
pythongh-111262: Add PyDict_Pop() function …
1498269 
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
@vstinner
Remove PyDict_Pop() from stable ABI
ae73490
@vstinner
sys_set_object() ignores removed value
f65c10f
@vstinner
Add PyDict_PopString() function
9da06c8
@vstinner
Make tests more explicit that NULL is returned if key is missing
343f75a
@vstinner vstinner enabled auto-merge (squash) November 14, 2023 12:19
@vstinner vstinner merged commit 4f04172 into python:main Nov 14, 2023
@vstinner vstinner deleted the dict_pop2 branch November 14, 2023 12:51
@vstinner vstinner mentioned this pull request Nov 14, 2023
Closed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@vstinner @scoder
@pitrou @aisk
pythongh-111262: Add PyDict_Pop() function (python#112028) …
024183c 
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@vstinner @scoder
@pitrou @Glyphack
pythongh-111262: Add PyDict_Pop() function (python#112028) …
99d4048 
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scoder scoder scoder approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@rhettinger rhettinger rhettinger is a code owner

@encukou encukou Awaiting requested review from encukou

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

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

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

Assignees

No one assigned

Labels

topic-C-API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vstinner @scoder @serhiy-storchaka