Issue 27786: longobject.c: simplify x_sub(), inline _PyLong_Negate() (original) (raw)

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

Messages (9)
msg272933 - (view)	Author: STINNER Victor (vstinner) *	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) *	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) *	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) *	Date: 2016-08-17 16:39
That works too. :)
msg272969 - (view)	Author: STINNER Victor (vstinner) *	Date: 2016-08-17 17:13
Updated patch.
msg272975 - (view)	Author: Serhiy Storchaka (serhiy.storchaka) *	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)	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) *	Date: 2016-08-17 17:50
Thanks for the review Brett, I pushed my fix.
msg272985 - (view)	Author: STINNER Victor (vstinner) *	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.