classification
Title: Add PyDict_GetItemSetDefault() as C-API for dict.setdefault()
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, pitrou, python-dev, rhettinger, scoder
Priority: normal Keywords: patch

Created on 2013-03-01 13:45 by scoder, last changed 2013-03-11 17:28 by scoder. This issue is now closed.

Files
File name Uploaded Description Edit
pydict_setitemdefault.patch scoder, 2013-03-01 13:45 implementation of new C-API function PyDict_GetItemSetDefault() review
pydict_setitemdefault.patch scoder, 2013-03-07 14:05 updated implementation of new C-API function PyDict_GetItemSetDefault() review
pydict_setitemdefault.patch scoder, 2013-03-07 23:27 updated implementation of new C-API function PyDict_SetDefault() review
Messages (24)
msg183260 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-01 13:45
The functionality of dict.setdefault() is not currently available through the C-API. Specfically, there is no way to test for a key and insert a fallback value for it without evaluating the hash function twice.

The attached patch adds a new C-API function PyDict_GetItemSetDefault() that makes this feature available. Like all PyDict_Getitem*() functions, it returns a borrowed reference.

I hope I got the update in refcounts.dat right. I have no idea how to express that a function *may* increase the refcounts of its arguments.
msg183638 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-03-07 08:10
+1 for the idea.

Please change "failobj" to "default" to keep the terminology consistent with the pure Python API.  For reference, here's the code from collections.MutableMapping:

    def setdefault(self, key, default=None):
        try:
            return self[key]
        except KeyError:
            self[key] = default
        return default
msg183662 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-07 14:05
The problem with 'default' is that it is a reserved word in C. I changed it to "defaultobj", except for the docs page, where "default" should work. I also removed the "register" declaration from the "mp" argument because it is most likely useless and just takes up screen real estate.
msg183695 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-07 19:01
Please call it PyDict_SetDefault, though.
msg183696 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-07 19:06
I had originally considered that name. However, what it really does is PyDict_GetItem(). In a specific special case, it sets a default value and *then* returns that. So it's still PyDict_GetItem(), just with a preceding modification. Also, the interface mimics PyDict_GetItem(). Its primary interface is a getter, not a setter.

I really think that PyDict_SetDefault() is an inappropriate name.
msg183697 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-07 19:12
PyDict_SetDefault mimicks the Python API, though.
msg183698 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-07 19:15
Well, guess what, I kind-of figured that. So, what's wrong with PyDict_GetItemSetDefault()? That mimics the Python method name, too, while at the same time making it clear what actually happens and how the C-API function behaves.
msg183699 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-03-07 19:18
I would simply call it PyDict_SetDefault, too. It's also shorter to type ;)
msg183700 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-07 19:21
To me, PyDict_SetDefault() sounds like it's supposed to set a default value that PyDict_GetItem() would return instead of NULL on lookup failure. Basically a defaultdict-like extension to normal dicts.
msg183701 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-07 19:25
I find the name PyDict_GetItemSetDefault very confusing. My brain isn't quite sure where to partition the four words of GetItemSetDefault. If you really wanted to be clear, you would have to call it PyDict_GetItemAndSetDefault. There's no reason not to pick the same name as the Python-level API, though.
msg183702 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-07 19:27
I'm fine with PyDict_GetItemOrSetDefault() as well.
msg183710 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-03-07 23:10
+1 for PyDict_SetDefault.  Any other name is confusing.
msg183713 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-07 23:27
Ok (shrug), since everyone seems to agree, PyDict_SetDefault() it is. I wouldn't be surprised if the same kind of discussion lead to the original naming of dict.setdefault()...
msg183723 - (view) Author: Roundup Robot (python-dev) Date: 2013-03-08 03:17
New changeset a0b750ea3397 by Benjamin Peterson in branch 'default':
Add PyDict_SetDefault. (closes #17327)
http://hg.python.org/cpython/rev/a0b750ea3397
msg183728 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-08 07:00
If you decide to refactor a well tested patch, you might want to give it another test run before committing it.

Objects/dictobject.c: In function 'dict_setdefault':
Objects/dictobject.c:2266:5: warning: passing argument 1 of 'PyDict_SetDefault' from incompatible pointer type [enabled by default]
Objects/dictobject.c:2215:1: note: expected 'struct PyObject *' but argument is of type 'struct PyDictObject *'
msg183729 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-08 07:48
I'm totally ok with your changes, though. The only real difference is an aditional type check in the slot function path, and that's not going to make any difference right after the costly operation of unpacking python function arguments.
msg183736 - (view) Author: Roundup Robot (python-dev) Date: 2013-03-08 13:36
New changeset ca9a85c36e09 by Benjamin Peterson in branch 'default':
fix warning (closes #17327)
http://hg.python.org/cpython/rev/ca9a85c36e09
msg183952 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-11 11:41
Benjamin, I hadn't noticed that you also changed the documentation in dict.rst from what I originall wrote. Ezio already fixed one of your typos, but there's another one in the description of the "defaultobj" behaviour: it says "inserted" twice but should say "returned" in the second case. Please give it another read to see if you introduced any further errors.
msg183971 - (view) Author: Roundup Robot (python-dev) Date: 2013-03-11 16:31
New changeset 89174df2ee45 by Benjamin Peterson in branch 'default':
remove useless words (#17327)
http://hg.python.org/cpython/rev/89174df2ee45
msg183974 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-11 16:44
Please use either of the following wordings in the documentation:

"""
If the key is not in the dict, it is inserted with value *defaultobj* and *defaultobj* is returned.
"""

or

"""
If the key is not in the dict, it is inserted with value *defaultobj*, which is then returned.
"""

Otherwise, the docs leave it open if anything at all is returned in the case of a lookup failure. (I initially wanted to make that clear with the PyDict_GetItem*() name, but since that was rejected, it's worth making up for in the docs.)
msg183975 - (view) Author: Roundup Robot (python-dev) Date: 2013-03-11 16:50
New changeset 8948dd77b095 by Benjamin Peterson in branch 'default':
say defaultobj is returned (#17327)
http://hg.python.org/cpython/rev/8948dd77b095
msg183977 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-11 16:56
... and it's called "defaultobj", not "defautobj".
msg183978 - (view) Author: Roundup Robot (python-dev) Date: 2013-03-11 17:17
New changeset f6f39ebd3121 by Benjamin Peterson in branch 'default':
fix spelling (#17327)
http://hg.python.org/cpython/rev/f6f39ebd3121
msg183980 - (view) Author: Stefan Behnel (scoder) * Date: 2013-03-11 17:28
Thanks!
History
Date User Action Args
2013-03-11 17:28:12scodersetmessages: + msg183980
2013-03-11 17:17:31python-devsetmessages: + msg183978
2013-03-11 16:56:16scodersetmessages: + msg183977
2013-03-11 16:50:35python-devsetmessages: + msg183975
2013-03-11 16:44:46scodersetmessages: + msg183974
2013-03-11 16:31:48python-devsetmessages: + msg183971
2013-03-11 11:41:05scodersetmessages: + msg183952
2013-03-08 13:36:55python-devsetstatus: open -> closed
resolution: fixed
messages: + msg183736
2013-03-08 07:48:54scodersetmessages: + msg183729
2013-03-08 07:00:27scodersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg183728
2013-03-08 03:17:26python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg183723

resolution: fixed
stage: resolved
2013-03-07 23:27:25scodersetfiles: + pydict_setitemdefault.patch

messages: + msg183713
2013-03-07 23:10:01rhettingersetmessages: + msg183710
2013-03-07 19:27:13scodersetmessages: + msg183702
2013-03-07 19:25:01benjamin.petersonsetmessages: + msg183701
2013-03-07 19:21:16scodersetmessages: + msg183700
2013-03-07 19:18:43pitrousetnosy: + pitrou
messages: + msg183699
2013-03-07 19:15:51scodersetmessages: + msg183698
2013-03-07 19:12:33benjamin.petersonsetmessages: + msg183697
2013-03-07 19:06:54scodersetmessages: + msg183696
2013-03-07 19:01:35benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg183695
2013-03-07 14:05:39scodersetfiles: + pydict_setitemdefault.patch

messages: + msg183662
2013-03-07 08:10:49rhettingersetnosy: + rhettinger
messages: + msg183638
2013-03-01 13:45:55scodercreate