gh-111262: Add PyDict_Pop() function [without default value nor KeyError] by vstinner · Pull Request #112028 · python/cpython · GitHub

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

Issue: Make _PyDict_Pop() public #111262

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

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

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]);
}

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

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.

Why not remove it?

I prefer to remove it in a separated PR: see #112026

Suggested change

      
                    result = Py_NewRef(Py_None);
          
                    return PyLong_FromLong(res);

I prefer to return (0, None) to make the tests written in Python closer to what the C API returns. In test_capi.test_dict, you can see that as (0, NULL).

It can be confused with actual None. For example dict.get() return an actual none, but several corresponding C API functions return NULL.

In other tests I made them returning AttributeError or KeyError, as it is less chance to confuse with real value, but I think that it would be better to use a special singleton _testcapi.MISSING in future.

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.

@serhiy-storchaka: I added more tests.

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.

LGTM overall, just wording fixes.

It's usually clearer to write "not raising an exception" (at all) than naming an explicit exception and leaving it open whether other exceptions might be raised in the described case.

Suggested change

      
            * Add :c:func:`PyDict_Pop` and :c:func:`PyDict_PopString` functions: remove a
          
              key from a dictionary and optionally return the removed value. This is the
          
              similar to :meth:`dict.pop`, but without the default value and do not raise
          
              :exc:`KeyError` if the key missing.
          
            * Add :c:func:`PyDict_Pop` and :c:func:`PyDict_PopString` functions: remove a
          
              key from a dictionary and optionally return the removed value. This is
          
              similar to :meth:`dict.pop`, but without the default value and not raising
          
              an exception if the key missing.

Thanks. I applied your suggestion to the 3 places where I wrote that doc, but I kept KeyError. IMO it's useful to be explicit about KeyError to understand what we are talking about.

@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.

_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>

_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 requested review from a team, encukou, kumaraditya303, markshannon, methane and rhettinger as code owners November 13, 2023 11:53

bedevere-app Bot added the awaiting core review label Nov 13, 2023

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

Make _PyDict_Pop() public #111262

Closed

vstinner added the topic-C-API label Nov 13, 2023

vstinner mentioned this pull request Nov 13, 2023

gh-111262: Add PyDict_Pop() function [with default value] #111939

Closed

vstinner changed the title gh-111262: Add PyDict_Pop() function gh-111262: Add PyDict_Pop() function [without default value nor KeyError] Nov 13, 2023

serhiy-storchaka reviewed Nov 13, 2023

View reviewed changes

Comment thread Lib/test/test_capi/test_dict.py

Comment thread Lib/test/test_capi/test_dict.py

Comment thread Lib/test/test_capi/test_dict.py

Comment thread Modules/_testcapi/dict.c Outdated

Comment thread Modules/_testcapi/dict.c

vstinner mentioned this pull request Nov 13, 2023

Return value conventions capi-workgroup/api-evolution#13

Open

vstinner force-pushed the dict_pop2 branch from fa618e6 to 7c84afb Compare November 13, 2023 23:04

serhiy-storchaka approved these changes Nov 14, 2023

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels Nov 14, 2023

scoder approved these changes Nov 14, 2023

View reviewed changes

vstinner and others added 5 commits November 14, 2023 13:19

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>

Remove PyDict_Pop() from stable ABI

ae73490

sys_set_object() ignores removed value

f65c10f

Add PyDict_PopString() function

9da06c8

Make tests more explicit that NULL is returned if key is missing

343f75a

vstinner added 4 commits November 14, 2023 13:19

Add tests

2170fc0

Enhance tests

c3b8bd2

Add more tests

a286604

Update doc

229f524

vstinner force-pushed the dict_pop2 branch from e988e07 to 229f524 Compare November 14, 2023 12:19

vstinner enabled auto-merge (squash) November 14, 2023 12:19

vstinner merged commit 4f04172 into python:main Nov 14, 2023

vstinner deleted the dict_pop2 branch November 14, 2023 12:51

bedevere-app Bot removed the awaiting merge label Nov 14, 2023

vstinner mentioned this pull request Nov 14, 2023

gh-111262: Make PyDict_Pop() public #111263

Closed



		PyObject *
		_PyDict_Pop(PyObject dict, PyObject key, PyObject *default_value)

+                      return NULL;
+                  }
+                  if (result == NULL) {
+                      result = Py_NewRef(Py_None);

gh-111262: Add PyDict_Pop() function [without default value nor KeyError] by vstinner · Pull Request #112028 · python/cpython · GitHub

Uh oh!

Uh oh!

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

gh-111262: Add PyDict_Pop() function [without default value nor KeyError]#112028vstinner merged 9 commits intopython:mainpython/cpython:mainfrom vstinner:dict_pop2vstinner/cpython:dict_pop2Copy head branch name to clipboard

Conversation

vstinner commented Nov 13, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

scoder Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Nov 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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