Title: Use Py_RETURN_NONE and like
Type: enhancement Stage: resolved
Components: Extension Modules, Interpreter Core Versions: Python 3.7
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: methane, python-dev, rhettinger, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2016-12-17 10:09 by serhiy.storchaka, last changed 2017-01-23 08:31 by serhiy.storchaka. This issue is now closed.

File name Uploaded Description Edit
py_return.cocci serhiy.storchaka, 2016-12-17 10:09 Coccinelle semantic patch
py_return.patch serhiy.storchaka, 2016-12-17 10:10 review
Messages (11)
msg283480 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-17 10:09
In the comment to issue28765 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.

msg283481 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-17 10:10
Resulting patch is huge. I don't think it can be pushed.
msg283483 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-17 10:34
py_return.patch LGTM.
msg283676 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2016-12-20 10:17
msg286005 - (view) Author: Inada Naoki (methane) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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
msg286058 - (view) Author: Roundup Robot (python-dev) (Python triager) 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
msg286059 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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.
Date User Action Args
2017-01-23 08:31:15serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg286059

stage: patch review -> resolved
2017-01-23 08:24:25python-devsetmessages: + msg286058
2017-01-23 07:47:49python-devsetnosy: + python-dev
messages: + msg286056
2017-01-23 05:53:53methanesetmessages: + msg286049
2017-01-23 05:47:50rhettingersetassignee: serhiy.storchaka

messages: + msg286047
nosy: + rhettinger
2017-01-22 12:53:18serhiy.storchakasetmessages: + msg286007
2017-01-22 12:22:42methanesetmessages: + msg286005
2016-12-20 10:17:28methanesetnosy: + methane
messages: + msg283676
2016-12-17 10:34:52vstinnersetmessages: + msg283483
2016-12-17 10:11:07serhiy.storchakasetfiles: + py_return.patch
keywords: + patch
messages: + msg283481

stage: patch review
2016-12-17 10:09:47serhiy.storchakacreate