classification
Title: Merge duplicated _Py_IDENTIFIER identifiers in C code
Type: Stage:
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: andrei.duma, fhahn, haypo, icordasc, loewis, python-dev, seydou
Priority: normal Keywords: easy, patch

Created on 2013-11-07 01:02 by haypo, last changed 2013-11-07 20:51 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
patch_19514.diff fhahn, 2013-11-07 15:51 review
merge_py_identifiers.patch andrei.duma, 2013-11-07 15:54 review
merge_py_identifiers_sorted.patch andrei.duma, 2013-11-07 16:01 review
Messages (13)
msg202300 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-07 01:02
Some C files use more than once the same _Py_IDENTIFIER identifier. It would be more efficient to merge duplicated identifiers. Just move the definition to the top of the file.

_Py_IDENTIFIER(as_integer_ratio): Modules/_datetimemodule.c:1569
_Py_IDENTIFIER(as_integer_ratio): Modules/_datetimemodule.c:1668

_Py_IDENTIFIER(cursor): Modules/_sqlite/connection.c:1282
_Py_IDENTIFIER(cursor): Modules/_sqlite/connection.c:1312
_Py_IDENTIFIER(cursor): Modules/_sqlite/connection.c:1342

_Py_IDENTIFIER(fromutc): Modules/_datetimemodule.c:4210
_Py_IDENTIFIER(fromutc): Modules/_datetimemodule.c:4249
_Py_IDENTIFIER(fromutc): Modules/_datetimemodule.c:4812

_Py_IDENTIFIER(__len__): Objects/typeobject.c:5071
_Py_IDENTIFIER(__len__): Objects/typeobject.c:5235

_Py_IDENTIFIER(insert): Modules/_bisectmodule.c:198
_Py_IDENTIFIER(insert): Modules/_bisectmodule.c:93

_Py_IDENTIFIER(isoformat): Modules/_datetimemodule.c:2638
_Py_IDENTIFIER(isoformat): Modules/_datetimemodule.c:3596
_Py_IDENTIFIER(isoformat): Modules/_datetimemodule.c:4532

_Py_IDENTIFIER(strftime): Modules/_datetimemodule.c:1280
_Py_IDENTIFIER(strftime): Modules/_datetimemodule.c:2679
msg202302 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-07 01:08
See also issue #19515 which is more general.
msg202308 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-07 07:47
Oh, I forgot:

_Py_IDENTIFIER(__delitem__): Objects/typeobject.c:5132
_Py_IDENTIFIER(__delitem__): Objects/typeobject.c:5183
msg202310 - (view) Author: Andrei Dorian Duma (andrei.duma) * Date: 2013-11-07 08:05
I'll provide a patch later tonight.
msg202347 - (view) Author: (fhahn) Date: 2013-11-07 15:51
I've merged the _Py_IDENTIFIER identifiers mentioned above.

I stumbled over anohter instance where _Py_IDENTIFIER is used more than once: 

_Py_IDENTIFIER(__setitem__) : Objects/typeobject.c#l5133
_Py_IDENTIFIER(__setitem__) : Objects/typeobject.c#l5184
msg202349 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2013-11-07 15:52
As a matter of style, I suggest that all identifiers are moved to the top of a file if some of them live there. IOW, it's (IMO) unstylish to have some at the top, and some in the middle (although this works perfectly fine, of course).
msg202350 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2013-11-07 15:53
Another matter of style: I suggest alphabetical order for the identifiers, at least when the list gets long.
msg202352 - (view) Author: Andrei Dorian Duma (andrei.duma) * Date: 2013-11-07 15:54
The patch I promised above.
msg202356 - (view) Author: Andrei Dorian Duma (andrei.duma) * Date: 2013-11-07 16:01
I added a new patch with sorted _Py_IDENTIFIERs.

Regarding all identifiers at the top, I guess it might be more stylish, but it might affect performance. I'm not sure, though.
msg202358 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-07 16:05
merge_py_identifiers_sorted.patch looks good to me.
msg202375 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-07 17:47
New changeset 4a09cc62419b by Martin v. Löwis in branch 'default':
Issue #19514: Deduplicate some _Py_IDENTIFIER declarations.
http://hg.python.org/cpython/rev/4a09cc62419b
msg202376 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2013-11-07 17:48
Thanks for the patch.

Note: moving all identifiers would not have made a difference. They are static variables, so from a run-time point of view, there is no difference whether they are inside or outside of functions.
msg202380 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-07 20:51
New changeset cb4c964800af by Victor Stinner in branch 'default':
Issue #19514: Add Andrei Dorian Duma to Misc/ACKS for changeset 4a09cc62419b
http://hg.python.org/cpython/rev/cb4c964800af
History
Date User Action Args
2013-11-07 20:51:13python-devsetmessages: + msg202380
2013-11-07 17:48:57loewissetstatus: open -> closed
resolution: fixed
messages: + msg202376
2013-11-07 17:47:09python-devsetnosy: + python-dev
messages: + msg202375
2013-11-07 16:05:39hayposetmessages: + msg202358
2013-11-07 16:01:58andrei.dumasetfiles: + merge_py_identifiers_sorted.patch

messages: + msg202356
2013-11-07 15:54:34andrei.dumasetfiles: + merge_py_identifiers.patch

messages: + msg202352
2013-11-07 15:53:22loewissetmessages: + msg202350
2013-11-07 15:52:42loewissetnosy: + loewis
messages: + msg202349
2013-11-07 15:51:25fhahnsetfiles: + patch_19514.diff

nosy: + fhahn
messages: + msg202347

keywords: + patch
2013-11-07 12:34:26icordascsetnosy: + icordasc
2013-11-07 08:13:53seydousetnosy: + seydou
2013-11-07 08:05:50andrei.dumasetmessages: + msg202310
2013-11-07 07:47:43hayposetmessages: + msg202308
2013-11-07 05:35:32andrei.dumasetnosy: + andrei.duma
2013-11-07 01:08:36hayposetmessages: + msg202302
2013-11-07 01:02:53haypocreate