Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spec: mistake regarding sign of x % y #148

Closed
alandonovan opened this issue Dec 9, 2020 · 1 comment
Closed

spec: mistake regarding sign of x % y #148

alandonovan opened this issue Dec 9, 2020 · 1 comment
Assignees

Comments

@alandonovan
Copy link
Contributor

alandonovan commented Dec 9, 2020

The spec currently 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.

yet the Go and Java implementations currently exhibit this behavior, as does Python[23]:

Welcome to Starlark (java.starlark.net)
>> maxlong = 0x7fffffffffffffff
>> minlong = -maxlong-1
>> 1 % minlong
-9223372036854775807

What's more, Python's language reference, which we have aimed to follow here, says:

https://docs.python.org/3/reference/expressions.html#binary-arithmetic-operations

The modulo operator always yields a result with the same sign as its second operand (or zero);

I think the text of our spec is wrong, and should be made to match Python. The implementations may have incorporated the mistake into their behavior, and will need auditing. D'oh.

(Context: bazelbuild/bazel#12667)

@stepancheg

@brandjon
Copy link
Member

brandjon commented Dec 9, 2020

Yep, all the wording I'm familiar with is that it gets the sign of the right-hand operand. You want the property that the result repeats periodically in the same way, even when x is at or about 0.

adonovan added a commit to adonovan/starlark-spec-fork that referenced this issue Jan 19, 2021
The Go and Java implementations agree with Python 2 and 3 that
the sign of x % y should match the divisor (y) not the dividend (x).
This change makes the spec match the implementations and the intent.

Fixes bazelbuild#148

Change-Id: Icc431c634bd768d7f626c98294ad854505d9da25
@alandonovan alandonovan self-assigned this Jan 19, 2021
adonovan added a commit to google/starlark-go that referenced this issue Jan 19, 2021
See bazelbuild/starlark#148
for change to Starlark spec.

Updates bazelbuild/starlark#148

Change-Id: I0135a125159e421f818a99785f22e082d3090f78
alandonovan pushed a commit to google/starlark-go that referenced this issue Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants