Skip to content

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

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

Merged
vstinner merged 9 commits into from
Nov 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1498269
gh-111262: Add PyDict_Pop() function
vstinner Nov 10, 2023
ae73490
Remove PyDict_Pop() from stable ABI
vstinner Nov 13, 2023
f65c10f
sys_set_object() ignores removed value
vstinner Nov 13, 2023
9da06c8
Add PyDict_PopString() function
vstinner Nov 13, 2023
343f75a
Make tests more explicit that NULL is returned if key is missing
vstinner Nov 13, 2023
2170fc0
Add tests
vstinner Nov 13, 2023
c3b8bd2
Enhance tests
vstinner Nov 13, 2023
a286604
Add more tests
vstinner Nov 13, 2023
229f524
Update doc
vstinner Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add tests
  • Loading branch information
@vstinner
vstinner committed Nov 14, 2023
commit 2170fc0b162b5dad691ddd3df9fe3bc5938ff7e1
43 changes: 39 additions & 4 deletions Lib/test/test_capi/test_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,21 +435,34 @@ def test_dict_mergefromseq2(self):
def test_dict_pop(self):
# Test PyDict_Pop()
dict_pop = _testcapi.dict_pop
dict_pop_null = _testcapi.dict_pop_null

# key present
# key present, get removed value
mydict = {"key": "value", "key2": "value2"}
self.assertEqual(dict_pop(mydict, "key"), (1, "value"))
self.assertEqual(mydict, {"key2": "value2"})
self.assertEqual(dict_pop(mydict, "key2"), (1, "value2"))
self.assertEqual(mydict, {})

# key missing; empty dict has a fast path
# key present, ignore removed value
mydict = {"key": "value", "key2": "value2"}
self.assertEqual(dict_pop_null(mydict, "key"), 1)
self.assertEqual(mydict, {"key2": "value2"})
self.assertEqual(dict_pop_null(mydict, "key2"), 1)
self.assertEqual(mydict, {})

# key missing, expect removed value; empty dict has a fast path
self.assertEqual(dict_pop({}, "key"), (0, NULL))
self.assertEqual(dict_pop({"a": 1}, "key"), (0, NULL))

# key missing, ignored removed value; empty dict has a fast path
self.assertEqual(dict_pop_null({}, "key"), 0)
self.assertEqual(dict_pop_null({"a": 1}, "key"), 0)

# dict error
not_dict = "string"
not_dict = UserDict({1: 2})
self.assertRaises(SystemError, dict_pop, not_dict, "key")
self.assertRaises(SystemError, dict_pop_null, not_dict, "key")

# key error; don't hash key if dict is empty
not_hashable_key = ["list"]
Expand All @@ -459,7 +472,29 @@ def test_dict_pop(self):
dict_pop({}, NULL) # key is not checked if dict is empty

# CRASHES dict_pop(NULL, "key")
# CRASHES dict_pop({"a": 1}, NULL, default)
# CRASHES dict_pop({"a": 1}, NULL)

def test_dict_popstring(self):
# Test PyDict_PopString()
dict_popstring = _testcapi.dict_popstring

# key present
mydict = {"key": "value", "key2": "value2"}
Comment thread
vstinner marked this conversation as resolved.
self.assertEqual(dict_popstring(mydict, "key"), (1, "value"))
self.assertEqual(mydict, {"key2": "value2"})
self.assertEqual(dict_popstring(mydict, "key2"), (1, "value2"))
self.assertEqual(mydict, {})

# key missing; empty dict has a fast path
self.assertEqual(dict_popstring({}, "key"), (0, NULL))
self.assertEqual(dict_popstring({"a": 1}, "key"), (0, NULL))

# dict error
not_dict = UserDict({1: 2})
self.assertRaises(SystemError, dict_popstring, not_dict, "key")
Comment thread
vstinner marked this conversation as resolved.

# CRASHES dict_popstring(NULL, "key")
# CRASHES dict_popstring({"a": 1}, NULL)
Comment thread
vstinner marked this conversation as resolved.


if __name__ == "__main__":
Expand Down
44 changes: 44 additions & 0 deletions Modules/_testcapi/dict.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ dict_mergefromseq2(PyObject *self, PyObject *args)
static PyObject *
dict_pop(PyObject *self, PyObject *args)
Comment thread
vstinner marked this conversation as resolved.
{
// Test PyDict_Pop(dict, key, &value)
PyObject *dict, *key;
if (!PyArg_ParseTuple(args, "OO", &dict, &key)) {
return NULL;
Expand All @@ -353,6 +354,47 @@ dict_pop(PyObject *self, PyObject *args)
}


static PyObject *
dict_pop_null(PyObject *self, PyObject *args)
{
// Test PyDict_Pop(dict, key, NULL)
PyObject *dict, *key;
if (!PyArg_ParseTuple(args, "OO", &dict, &key)) {
return NULL;
}
NULLABLE(dict);
NULLABLE(key);
int res = PyDict_Pop(dict, key, NULL);
if (res < 0) {
Comment thread
vstinner marked this conversation as resolved.
Outdated
return NULL;
}
return PyLong_FromLong(res);
}


static PyObject *
dict_popstring(PyObject *self, PyObject *args)
Comment thread
vstinner marked this conversation as resolved.
{
PyObject *dict;
const char *key;
Py_ssize_t key_size;
if (!PyArg_ParseTuple(args, "Oz#", &dict, &key, &key_size)) {
return NULL;
}
NULLABLE(dict);
PyObject *result = UNINITIALIZED_PTR;
int res = PyDict_PopString(dict, key, &result);
if (res < 0) {
assert(result == NULL);
return NULL;
}
if (result == NULL) {
result = Py_NewRef(Py_None);
}
return Py_BuildValue("iN", res, result);
}


static PyMethodDef test_methods[] = {
{"dict_check", dict_check, METH_O},
{"dict_checkexact", dict_checkexact, METH_O},
Expand Down Expand Up @@ -381,6 +423,8 @@ static PyMethodDef test_methods[] = {
{"dict_update", dict_update, METH_VARARGS},
{"dict_mergefromseq2", dict_mergefromseq2, METH_VARARGS},
{"dict_pop", dict_pop, METH_VARARGS},
{"dict_pop_null", dict_pop_null, METH_VARARGS},
{"dict_popstring", dict_popstring, METH_VARARGS},
{NULL},
};

Expand Down