| msg283480 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2016-12-17 10:09 |
| In the comment to patch Victor suggested to replace "Py_INCREF(Py_None); return Py_None;" with "Py_RETURN_NONE;". Here is a Coccinelle [1] semantic patch that replaces all returns of new references to None, True or False with macros Py_RETURN_NONE, Py_RETURN_TRUE or Py_RETURN_FALSE correspondingly. [1] http://coccinelle.lip6.fr/ |
|
|
| msg283481 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-12-17 10:10 |
| Resulting patch is huge. I don't think it can be pushed. |
|
|
| msg283483 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2016-12-17 10:34 |
| py_return.patch LGTM. |
|
|
| msg283676 - (view) |
Author: Inada Naoki (methane) * |
Date: 2016-12-20 10:17 |
| LGTM |
|
|
| msg286005 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-01-22 12:22 |
| While patch looks safe, some developer may dislike such a large patch without fixing real issue. Anyway, Coccinelle seems very interesting tool for refactoring. |
|
|
| msg286007 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-01-22 12:53 |
| That is why I haven't pushed the patch. Thanks you for your LGTMs Victor and Inada, but this is not enough. In general I think that it is better to use Py_RETURN_NONE than "Py_INCREF(Py_None); return Py_None;". Some replacements already was done in the past, and some currently reviewed patches include also such changes. This can distract attention from the main purpose of patches. I think that pushing all changes in one big commit will make less harm than add small changes in other patches here and there. Concinelle proved his reliability and it is easy to check these changes. I'll push this patch if Raymond or other more conservative core developers accept it (or selected parts of it). |
|
|
| msg286047 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2017-01-23 05:47 |
| I believe this is a safe change. All of the replaced pairs on consecutive lines so there are no intervening operations. For Modules/xxsubtype.c, I think the code should remain as-is. It is intended to be the simplest possible example of how to make a subtype and its clarity is reduced by requiring that someone learn the macro. That said, it would be reasonable to add a comment that this pair could also have been coded as Py_RETURN_NONE (i.e. use it as a teachable moment). |
|
|
| msg286049 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-01-23 05:53 |
| Oh, I feel three LGTMs are positive signal. As I commented on ML, I think "ask forgiveness than permission" is realistic approach for patches like this. But it's up to you. |
|
|
| msg286056 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2017-01-23 07:47 |
| New changeset df87db35833e by Serhiy Storchaka in branch 'default': Issue #28999: Use Py_RETURN_NONE, Py_RETURN_TRUE and Py_RETURN_FALSE wherever https://hg.python.org/cpython/rev/df87db35833e |
|
|
| msg286058 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2017-01-23 08:24 |
| New changeset 2c724f45f23f by Serhiy Storchaka in branch 'default': Issue #28999: Use Py_RETURN_NONE, Py_RETURN_TRUE and Py_RETURN_FALSE wherever https://hg.python.org/cpython/rev/2c724f45f23f |
|
|
| msg286059 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-01-23 08:31 |
| Thank you Raymond. I didn't expect to get an approval from you and were ready to withdraw the patch. I myself were just +0 on pushing these changes. Omitted changes in Modules/xx*.c and added few changes which Coccinelle missed. |
|
|