| msg188531 - (view) |
Author: Charles-François Natali (neologix) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2013-05-16 20:58 |
| ins_macro-2.diff looks good to me, go ahead! |
|
|
| msg189677 - (view) |
Author: Roundup Robot (python-dev)  |
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) * |
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. |
|
|