asm-js / validator

A reference validator for asm.js.
Apache License 2.0
1.78k stars 148 forks source link

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

Closed jruderman closed 10 years ago

jruderman commented 11 years ago

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

See:

cscott commented 11 years ago

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 commented 11 years ago

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

cscott commented 11 years ago

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 commented 11 years ago

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

cscott commented 11 years ago

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

sunfishcode commented 10 years ago

Fixed in 7881fbe325fbfbeed1caf5d1d48b8c5f121ead2d.