bpo-36027: Extend three-argument pow to negative second argument #13266
Conversation
| /* if exponent is negative, negate the exponent and | ||
| replace the base with a modular inverse */ | ||
| if (Py_SIZE(b) < 0) { | ||
| temp = (PyLongObject *)_PyLong_Copy(b); |
There was a problem hiding this comment.
I think Py_SETREF() should be used in this block.
There was a problem hiding this comment.
I'd rather keep it the way it is in this PR, for consistency with the existing previous block that negates the modulus. I'm happy to make a separate PR that updates both blocks (and other uses in longobject.c) to use Py_SETREF, but I don't want the long_pow code to be doing the same thing in two different ways in two places, and I want to keep this particular PR focused on the inversion feature.
|
I don't quite like the wording in the documentation as this isn't about the |
|
@jdemeyer Agreed. I've updated the wording to make it clearer that this feature applies only for |
…ltin, remove accidental spacing change.
Doc/library/functions.rst
Outdated
|
|
||
| For :class:`int` operands *x* and *y*, if *z* is present, *z* must also be | ||
| of integer type and *z* must be nonzero. If *z* is present and *y* is | ||
| negative, *x* must be relatively prime to *z*. In that case, ``pow(ix, -y, |
There was a problem hiding this comment.
Consider changing ix to inv_x for readability.
Doc/library/functions.rst
Outdated
| For :class:`int` operands *x* and *y*, if *z* is present, *z* must also be | ||
| of integer type and *z* must be nonzero. If *z* is present and *y* is | ||
| negative, *x* must be relatively prime to *z*. In that case, ``pow(ix, -y, | ||
| z)`` is returned, where *ix* is an inverse to *x* modulo *z*. |
There was a problem hiding this comment.
Perhaps this needs an example for clarity:
# Multiplicative inverse of 38 in mod 97
>>> pow(38, -1, 97)
23
>>> 23 * 38 % 97 == 1
True
| /* Compute an inverse to a modulo n, or raise ValueError if a is not | ||
| invertible modulo n. Assumes n is positive. The inverse returned | ||
| is whatever falls out of the extended Euclidean algorithm: it may | ||
| be either positive or negative, but will be smaller than n in |
There was a problem hiding this comment.
Oh my, the "it may be either positive or negative" is alarming!
When I first read this, I feared you might have used the result of this utility function directly, making pow(5, -1, 8) return -3.
I see you didn't, but maybe a word of reassurance here would be helpful!
| } | ||
| Py_DECREF(a); | ||
| a = n; | ||
| n = r; |
There was a problem hiding this comment.
Seems like you could skip the final round of arithmetic (the long_mul and long_sub) used to update c, by leaving the loop early at this point if the remainder is 0, right?
Just make sure to update b on the way out.
This PR allows the built-in
powto compute modular inverses, by providing a negative second argument:https://bugs.python.org/issue36027