gh-143006: Fix and optimize mixed comparison of float and int #143084
Conversation
When comparing negative non-integer float and int with the same number of bits in the integer part, __neg__() in the int subclass returning not an int caused an assertion error. Now the integer is no longer negated. Also, reduced the number of temporary created Python objects.
serhiy-storchaka
added
needs backport to 3.13
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
This approach looks better for me. Few minor nits.
| /* Convert w to a double if it fits. In particular, 0 fits. */ | ||
| int64_t nbits64 = _PyLong_NumBits(w); | ||
| assert(nbits64 >= 0); | ||
| assert(!PyErr_Occurred()); |
There was a problem hiding this comment.
It checks that _PyLong_NumBits() does not fail. It newer fails now.
Objects/floatobject.c
Outdated
| if (vsign < 0) { | ||
| op = Py_GT; // v >= w <=> trunc(v) > w | ||
| } |
There was a problem hiding this comment.
Perhaps, it might worth to do sign canonicalization, as before, to get rid of such cases. But I think we have to expose long_neg() for this.
There was a problem hiding this comment.
It will only remove 2 of 6 cases and simplify other 2. The code for negation of both arguments is more complex.
We could also use two tables, this would make the code smaller: op = table[vsign > 0][op].
There was a problem hiding this comment.
The idea was not just to reduce number of lines, but make algorithm more clear. Though, now I'm not sure we gain too much. I'm happy with current code.
|
@tim-one, could you please look at it? It was originally your code, 307fa78 (bpo-513866/gh-36040). |
|
CC @vstinner, maybe you could look too? |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM. The change removes the PyNumber_Negative() call.
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…ythonGH-143084) When comparing negative non-integer float and int with the same number of bits in the integer part, __neg__() in the int subclass returning not an int caused an assertion error. Now the integer is no longer negated. Also, reduced the number of temporary created Python objects. (cherry picked from commit 66bca383bd3b12d21e879d991d77b37a4c638f88) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
|
GH-143623 is a backport of this pull request to the 3.14 branch. |
…d int (pythonGH-143084) When comparing negative non-integer float and int with the same number of bits in the integer part, __neg__() in the int subclass returning not an int caused an assertion error. Now the integer is no longer negated. Also, reduced the number of temporary created Python objects. (cherry picked from commit 66bca38) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-143624 is a backport of this pull request to the 3.13 branch. |
…H-143084) (GH-143623) When comparing negative non-integer float and int with the same number of bits in the integer part, __neg__() in the int subclass returning not an int caused an assertion error. Now the integer is no longer negated. Also, reduced the number of temporary created Python objects. (cherry picked from commit 66bca38) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…H-143084) (GH-143624) When comparing negative non-integer float and int with the same number of bits in the integer part, __neg__() in the int subclass returning not an int caused an assertion error. Now the integer is no longer negated. Also, reduced the number of temporary created Python objects. (cherry picked from commit 66bca38)
3 participants
When comparing negative non-integer float and int with the same number of bits in the integer part,
__neg__()in the int subclass returning not an int caused an assertion error.Now the integer is no longer negated. Also, reduced the number of temporary created Python objects.
float_richcomparevia re-entrant__neg__#143006