classification
Title: use PyModule_AddIntMacro() instead of PyModule_AddIntConstant() when applicable
Type: enhancement Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: neologix, pitrou, python-dev, tshepang, vstinner
Priority: low Keywords: easy, needs review, patch

Created on 2013-05-06 13:10 by neologix, last changed 2013-05-21 16:45 by tshepang. This issue is now closed.

Files
File name Uploaded Description Edit
ins_macro-1.diff neologix, 2013-05-06 21:20 review
ins_macro-2.diff neologix, 2013-05-07 16:48 review
Messages (11)
msg188531 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-05-06 13:10
In many cases, PyModule_AddIntMacro() could be used instead of PyModule_AddIntConstant(), e.g. in socketmodule.c and posixmodule.c:

PyModule_AddIntMacro(m, AF_INET6);

vs (currently)

PyModule_AddIntConstant(m, "AF_INET6", AF_INET6);

It reduces the possibility of typo and is less verbose.
msg188576 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-05-06 19:48
Here's a (gigantic) patch.
I used an ad-hoc script for the conversion (next time I might try with coccinelle).

I tested it on Linux, FreeBSD, Openindiana, OS-X and Windows.
msg188580 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-06 19:57
This is a lot of code churn, but it touches code that is unlikely to be modified otherwise, so I guess it's ok.
msg188593 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-05-06 21:07
Could you please generete the same patch without --git so it can be reviewed on Rietveld?
msg188603 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-05-06 22:22
Modules/fcntlmodule.c and Modules/posixmodule.c are using explicit cast to long. I don't know if there is a good reason for such cast.

PC/_msi.c: Oh, here you should remove cast to int. Example:
 	
PyModule_AddIntMacro(m, (int)MSIDBOPEN_CREATEDIRECT);

I'm surprised that the does compile. You may have a "(int)MSIDBOPEN_CREATEDIRECT" variable :-)
msg188625 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-05-07 07:14
> PC/_msi.c: Oh, here you should remove cast to int. Example:
>
> PyModule_AddIntMacro(m, (int)MSIDBOPEN_CREATEDIRECT);
>
> I'm surprised that the does compile. You may have a "(int)MSIDBOPEN_CREATEDIRECT" variable :-)

Probably, good catch ;-)
I'll fix that.

> Modules/fcntlmodule.c and Modules/posixmodule.c are using explicit cast to long. I don't know if there is a good reason for such cast.

There's a prototype, so arguments are implicitly converted to long:

PyAPI_FUNC(int) PyModule_AddIntConstant(PyObject *, const char *, long);
#define PyModule_AddIntMacro(m, c) PyModule_AddIntConstant(m, #c, c)
msg189412 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-05-16 20:58
ins_macro-2.diff looks good to me, go ahead!
msg189677 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-05-20 17:14
New changeset 12cbb5183d98 by Charles-Francois Natali in branch 'default':
Issue #17917: Use PyModule_AddIntMacro() instead of PyModule_AddIntConstant()
http://hg.python.org/cpython/rev/12cbb5183d98
msg189736 - (view) Author: Tshepang Lekhonkhobe (tshepang) * Date: 2013-05-21 09:46
@antoine
I don't understand "This is a lot of code churn, but it touches code that is unlikely to be modified otherwise, so I guess it's ok.". What does it mean it's okay when it touches on code that's unlikely to be modified?
msg189738 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-05-21 09:53
> I don't understand "This is a lot of code churn, but it touches code that is unlikely to be modified otherwise, so I guess it's ok.". What does it mean it's okay when it touches on code that's unlikely to be modified?

The problem with refactoring is that it can complicate further merges
between branches. In this case, the code is nearly never touched, so
it's unlikely to be an issue.
msg189772 - (view) Author: Tshepang Lekhonkhobe (tshepang) * Date: 2013-05-21 16:45
Ok, I thought so. Just wanted to make sure.
History
Date User Action Args
2013-05-21 16:45:42tshepangsetmessages: + msg189772
2013-05-21 09:53:19neologixsetmessages: + msg189738
2013-05-21 09:46:45tshepangsetnosy: + tshepang
messages: + msg189736
2013-05-21 09:07:58neologixsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2013-05-20 17:14:04python-devsetnosy: + python-dev
messages: + msg189677
2013-05-16 20:58:10vstinnersetmessages: + msg189412
2013-05-07 16:48:27neologixsetfiles: + ins_macro-2.diff
2013-05-07 07:14:18neologixsetmessages: + msg188625
2013-05-06 22:22:43vstinnersetmessages: + msg188603
2013-05-06 21:20:57neologixsetfiles: + ins_macro-1.diff
2013-05-06 21:20:22neologixsetfiles: - ins_macro.diff
2013-05-06 21:07:01vstinnersetmessages: + msg188593
2013-05-06 19:57:33pitrousetmessages: + msg188580
2013-05-06 19:49:44pitrousetnosy: + vstinner
2013-05-06 19:48:53neologixsetfiles: + ins_macro.diff
2013-05-06 19:48:35neologixsetnosy: + pitrou
messages: + msg188576

keywords: + patch, needs review
stage: needs patch -> patch review
2013-05-06 13:10:37neologixcreate