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

integer modulus (%) should be intish, not int #69

Closed
jruderman opened this Issue Jun 11, 2013 · 6 comments

Comments

Projects
None yet
3 participants
@jruderman

jruderman commented Jun 11, 2013

(x%0) is NaN in JavaScript, so you don't want to use it directly as an int.

See:

@cscott

This comment has been minimized.

Show comment
Hide comment
@cscott

cscott Jun 12, 2013

To be concrete: the spec should give the same type for / and % in section 8.2 "binary operators": (signed, signed) → intish ∧ (unsigned, unsigned) → intish ∧ (doublish, doublish) → double

cscott commented Jun 12, 2013

To be concrete: the spec should give the same type for / and % in section 8.2 "binary operators": (signed, signed) → intish ∧ (unsigned, unsigned) → intish ∧ (doublish, doublish) → double

@jruderman

This comment has been minimized.

Show comment
Hide comment
@jruderman

jruderman Jun 12, 2013

Since this makes / and % have the same type, they can be collapsed into one table row.

jruderman commented Jun 12, 2013

Since this makes / and % have the same type, they can be collapsed into one table row.

@cscott

This comment has been minimized.

Show comment
Hide comment
@cscott

cscott Jun 12, 2013

In https://bugzilla.mozilla.org/show_bug.cgi?id=878433#c9 I propose also adding the following in section 6.6.8:

A MultiplicativeExpression node

expr:MultiplicativeExpression % n:NumericLiteral

validates as type int if n is nonzero and validates as a subtype of signed and expr
validates as a subtype of signed, or else if n is nonzero and validates as a subtype
of unsigned and expr validates as a subtype of unsigned.

This reduces the number of casts required for the common cases. (See bugzilla for more discussion.)

cscott commented Jun 12, 2013

In https://bugzilla.mozilla.org/show_bug.cgi?id=878433#c9 I propose also adding the following in section 6.6.8:

A MultiplicativeExpression node

expr:MultiplicativeExpression % n:NumericLiteral

validates as type int if n is nonzero and validates as a subtype of signed and expr
validates as a subtype of signed, or else if n is nonzero and validates as a subtype
of unsigned and expr validates as a subtype of unsigned.

This reduces the number of casts required for the common cases. (See bugzilla for more discussion.)

@jruderman

This comment has been minimized.

Show comment
Hide comment
@jruderman

jruderman Jun 12, 2013

Perhaps. asm.js compilers have to do a lot of cast-eliding optimization anyway.

jruderman commented Jun 12, 2013

Perhaps. asm.js compilers have to do a lot of cast-eliding optimization anyway.

@cscott

This comment has been minimized.

Show comment
Hide comment
@cscott

cscott Jun 15, 2013

@jruderman yes, but the extra casts also bloat code/download size, so they are desirable to remove from the source if possible.

cscott commented Jun 15, 2013

@jruderman yes, but the extra casts also bloat code/download size, so they are desirable to remove from the source if possible.

@sunfishcode

This comment has been minimized.

Show comment
Hide comment
@sunfishcode

sunfishcode Jun 3, 2014

Collaborator

Fixed in 7881fbe.

Collaborator

sunfishcode commented Jun 3, 2014

Fixed in 7881fbe.

@sunfishcode sunfishcode closed this Jun 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment