classification
Title: Convert OrderedDict methods to Argument Clinic
Type: performance Stage:
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, haypo, inada.naoki, python-dev, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-01-16 23:24 by haypo, last changed 2017-02-01 16:18 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
odict_clinic.patch haypo, 2017-01-16 23:24 review
Messages (12)
msg285588 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-16 23:24
Attached patch converts methods using METH_VARARGS|METH_KEYWORDS calling convention to METH_FASTCALL using Argument Clinic. This calling convention is faster, and Argument Clinic provides better docstring.

See also issue #29263 "Implement LOAD_METHOD/CALL_METHOD for C functions" and issue #29286 "Use METH_FASTCALL in str methods".
msg285589 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-16 23:26
Converted methods:

* fromkeys() (@classmethod)
* setdefault()
* pop()
* popitem()
* move_to_end()
msg285598 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-01-17 01:38
This patch segfaults for me (Mac OS 10.12.2 with the default clang-800.0.42.1).

For now, pop() should be omitted.  The argument clinic doesn't cope well with optional arguments without a default value.  The generated help "pop(self, /, key, default=None)" doesn't really make since because the default is not None, it is a sentinel value.  The generated help incorrectly makes it look like OrderedDict.get() which actually does have a default=None.
msg285601 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-01-17 02:26
LGTM, except pop().
msg285604 - (view) Author: Roundup Robot (python-dev) Date: 2017-01-17 02:47
New changeset 9f7d16266928 by Victor Stinner in branch 'default':
Convert some OrderedDict methods to Argument Clinic
https://hg.python.org/cpython/rev/9f7d16266928
msg285605 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-17 02:48
Raymond: " The argument clinic doesn't cope well with optional arguments without a default value."

Right, it's a bug in AC. I will try to fix it later. In the meanwhile, I pushed the first uncontroversal part.
msg285606 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-17 02:58
Oh, I missed Naoki's comment on the review :-/ I got the notification of this review after I pushed the change. I will fix the docstring later.
msg285652 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-17 15:18
For OrderedDict.pop(), I created the issue #29299: "Argument Clinic: Fix signature of optional positional-only arguments".
msg285655 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-17 15:21
I have added other comments on Rietveld (mostly about docstrings).
msg285820 - (view) Author: Roundup Robot (python-dev) Date: 2017-01-19 17:38
New changeset c144bf6c0ff7 by Serhiy Storchaka in branch 'default':
Issue #29289: Argument Clinic generates reasonable name for the parameter "default".
https://hg.python.org/cpython/rev/c144bf6c0ff7
msg286220 - (view) Author: Roundup Robot (python-dev) Date: 2017-01-24 22:30
New changeset ffc0840762e4 by Serhiy Storchaka in branch 'default':
Issues #29311, #29289: Fixed and improved docstrings for dict and OrderedDict
https://hg.python.org/cpython/rev/ffc0840762e4
msg286646 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-01 16:18
Thanks again Serhiy for the better docstrings!

Known issues are now fixed, so I close the issue. Thanks for the review Naoki.
History
Date User Action Args
2017-02-01 16:18:43hayposetstatus: open -> closed
resolution: fixed
messages: + msg286646
2017-01-24 22:30:33python-devsetmessages: + msg286220
2017-01-19 17:38:35python-devsetmessages: + msg285820
2017-01-17 15:21:28serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg285655
2017-01-17 15:18:58hayposetmessages: + msg285652
2017-01-17 02:58:49hayposetmessages: + msg285606
2017-01-17 02:48:40hayposetmessages: + msg285605
2017-01-17 02:47:53python-devsetnosy: + python-dev
messages: + msg285604
2017-01-17 02:26:34inada.naokisetmessages: + msg285601
2017-01-17 01:38:17rhettingersetnosy: + rhettinger
messages: + msg285598
2017-01-16 23:26:08hayposetmessages: + msg285589
2017-01-16 23:24:44haypocreate