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

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

Merged
merged 9 commits into from
Nov 14, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 13, 2023

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


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

@vstinner
Copy link
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 gh-111262: Add PyDict_Pop() function [without default value nor KeyError] Nov 13, 2023
@vstinner
Copy link
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
Copy link
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.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

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.

Objects/structseq.c Outdated Show resolved Hide resolved
Objects/structseq.c Outdated Show resolved Hide resolved
Python/sysmodule.c Outdated Show resolved Hide resolved
}


PyObject *
_PyDict_Pop(PyObject *dict, PyObject *key, PyObject *default_value)
Copy link
Member

Choose a reason for hiding this comment

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

Why not remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Modules/socketmodule.c Outdated Show resolved Hide resolved
Modules/_testcapi/dict.c Show resolved Hide resolved
return NULL;
}
if (result == NULL) {
result = Py_NewRef(Py_None);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Modules/_testcapi/dict.c Show resolved Hide resolved
Lib/test/test_capi/test_dict.py Outdated Show resolved Hide resolved
Lib/test/test_capi/test_dict.py Outdated Show resolved Hide resolved
@vstinner
Copy link
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.

Lib/test/test_capi/test_dict.py Show resolved Hide resolved
Lib/test/test_capi/test_dict.py Show resolved Hide resolved
Lib/test/test_capi/test_dict.py Show resolved Hide resolved
Modules/_testcapi/dict.c Outdated Show resolved Hide resolved
Modules/_testcapi/dict.c Show resolved Hide resolved
@vstinner
Copy link
Member Author

@serhiy-storchaka: I added more tests.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

LGTM overall, just wording fixes.

Comment on lines 1171 to 1181
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@vstinner
Copy link
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 vstinner enabled auto-merge (squash) November 14, 2023 12:19
@vstinner vstinner merged commit 4f04172 into python:main Nov 14, 2023
30 checks passed
@vstinner vstinner deleted the dict_pop2 branch November 14, 2023 12:51
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants