This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: longobject.c: simplify x_sub(), inline _PyLong_Negate()
Type: Stage:
Components: Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Oren Milman, brett.cannon, mark.dickinson, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2016-08-17 13:17 by vstinner, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
x_sub.patch vstinner, 2016-08-17 13:17
x_sub-2.patch vstinner, 2016-08-17 17:13 review
Messages (9)
msg272933 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-17 13:17
When reading the issue #27725, I saw that x_sub() of Objects/longobject.c is too careful just to change the sign of the newly created number: z cannot be shared at the end of the function, before z is normalized.

Attached patch simplifies the code.

See also the issue #27073, another similar simplification.
msg272965 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-17 16:30
I would add a comment as to why the assertion is there, otherwise it seems somewhat random that it exists.
msg272966 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-17 16:33
> I would add a comment as to why the assertion is there, otherwise it seems somewhat random that it exists.

Hum. Maybe it's even better to remove the assertion :-)
msg272967 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-17 16:39
That works too. :)
msg272969 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-17 17:13
Updated patch.
msg272975 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-17 17:48
LGTM.

Maybe use size_a instead of Py_SIZE(z)?

And look at "sign". Currently it takes values 1 and -1. Is it worth to replace it with boolean variable "negative"? Or Py_ssize_t variable size_z that takes values size_a and -size_a?
msg272976 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-17 17:49
New changeset be9dc240bf28 by Victor Stinner in branch 'default':
Issue #27786: Simplify x_sub()
https://hg.python.org/cpython/rev/be9dc240bf28
msg272978 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-17 17:50
Thanks for the review Brett, I pushed my fix.
msg272985 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-17 21:13
Serhiy Storchaka: "LGTM."

Oh, you posted your comment while I was pushing the patch after Brett wrote LGTM on the review (not on the bug tracker).

> Maybe use size_a instead of Py_SIZE(z)?
> And look at "sign". Currently it takes values 1 and -1. Is it worth to replace it with boolean variable "negative"? Or Py_ssize_t variable size_z that takes values size_a and -size_a?

Hum, I don't know what is the best. I don't think that it has an impact on performance. Feel free to modify directly the code, your proposed changes look safe and simple enough.
History
Date User Action Args
2022-04-11 14:58:34adminsetgithub: 71973
2016-08-17 21:13:53vstinnersetmessages: + msg272985
2016-08-17 17:50:46vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg272978
2016-08-17 17:49:05python-devsetnosy: + python-dev
messages: + msg272976
2016-08-17 17:48:49serhiy.storchakasetmessages: + msg272975
2016-08-17 17:13:59vstinnersetfiles: + x_sub-2.patch

messages: + msg272969
2016-08-17 16:39:26brett.cannonsetmessages: + msg272967
2016-08-17 16:33:08vstinnersetmessages: + msg272966
2016-08-17 16:30:19brett.cannonsetnosy: + brett.cannon
messages: + msg272965
2016-08-17 13:17:18vstinnersetnosy: + Oren Milman
2016-08-17 13:17:12vstinnercreate