Starlark: StarlarkInt.{floordiv,mod} now work with 64-bit integers #12667
Conversation
| @@ -136,6 +136,10 @@ assert_eq( product % 1234567, 1013598) | |||
| assert_eq( product % -1234567, -220969) # ditto | |||
| assert_eq(-product % 1234567, 220969) # ditto | |||
| assert_eq(-product % -1234567, -1013598) | |||
| assert_eq(1 % maxlong, 1) | |||
| assert_eq((-1) % maxlong, maxlong - 1) | |||
| assert_eq(1 % minlong, maxlong) | |||
There was a problem hiding this comment.
This is odd. Python3, go.starlark.net, and java.starlark.net (without this PR) all return -maxlong for this operation. Yet the spec says:
The // and % operations on integers compute floored division and remainder of floored division, respectively. If the signs of the operands differ, the sign of the remainder x % y matches that of the dividend, x.
This means the result should be maxlong, like you have here. Do all three implementations currently have the same bug? Or is the spec missing some subtlety?
There was a problem hiding this comment.
I think the spec is wrong. See bazelbuild/starlark#148
There was a problem hiding this comment.
To add more confusion, this is a failing assertion, it won't fail if:
assert_eq(1 % minlong, -maxlong)
with this PR implementation. Accident when I submitted the PR.
There was a problem hiding this comment.
(When I added tests, I tried to follow Python behavior, not the spec, but somehow made a mistake.)
* No special case for 32-bit integers, division is slow anyway * Switch `mod` to use `Math.floorMod` for simplicity/safety
modto useMath.floorModfor simplicity/safety