feat: implements almost all of the i32 operations #100
Conversation
|
updated to add:
|
|
updated to add:
|
|
| @@ -79,52 +79,52 @@ it("should conform to the wasm test suite", () => { | |||
| equals(m.exports.div_u(11, 5), 2); | |||
| equals(m.exports.div_u(17, 7), 2); | |||
|
|
|||
| // throws(() => m.exports.rem_s(1, 0), "$4"); | |||
There was a problem hiding this comment.
Could we remove it instead of commenting it?
|
|
||
| export function createValueFromAST(value: number): StackLocal { | ||
| value = (value | 0) % Math.pow(2, bits); | ||
| export class i32 implements Number<i32> { |
There was a problem hiding this comment.
You probably need to rebase, I had to rename the interface.
There was a problem hiding this comment.
Ah - good spot, I've turned Flow back on again, and fixed the issues this PR introduced :-)
|
Looks nice! I will review it in detail on my desktop tommorow. |
NOTE: these are simple, worst case O(31), implementations. In future, it might be worth using a more optimal algorithm http://graphics.stanford.edu/~seander/bithacks.html#ZerosOnRightLinear |
ColinEberhardt
changed the title
| @@ -69,6 +70,92 @@ export class BaseNumber implements NumberInterface<BaseNumber> { | |||
| return new BaseNumber(-this._value); | |||
| } | |||
|
|
|||
| /*eslint-disable no-unused-vars*/ | |||
There was a problem hiding this comment.
Just FYI I commented the operand somewhere in a PR, like:
rem_s(/*operand: BaseNumber*/): BaseNumber {
throw new RuntimeError("Unsupported operation");
}There was a problem hiding this comment.
Ah, good point - for some reason I thought doing that would result in Flow errors, but it does not :-)
| return createValue(c.popcnt()); | ||
|
|
||
| case "eqz": | ||
| return createValue(c.eqz()); |
There was a problem hiding this comment.
By the way, why is the value called c? value sounds more explicit to me.
There was a problem hiding this comment.
good point - I'll change that.
| } | ||
|
|
||
| isTrue(): boolean { | ||
| return this._value == 1; |
There was a problem hiding this comment.
We can use triple equal here. I'm not sure about this tbh, numeric types are not booleans, either the name is misleading or we could inline the === 1 in the callers?
There was a problem hiding this comment.
=== 👍
This reflects the specification which details a boolean interpretation for integers:
https://webassembly.github.io/spec/core/exec/numerics.html#boolean-interpretation
I'll add this as a comment.
| (i32.ge_u) | ||
| ) | ||
| (export "ge_u" (func $ge_u)) | ||
| ) |
There was a problem hiding this comment.
I really like that, the spec is way more testable this way, but way less maintainable.
We can commit that we after we include the spec tests we should remove it, ok? If so I can create an issue to track it
There was a problem hiding this comment.
I'll add an issue to track
progress towards #97
fixes #87
I'm basically running through all of our integer operations with a goal of getting them fully spec compliant and closing off #97
I've taken the official webassembly test suite and used a few regexes to convert it into chai / mocha test. Once #76 is implemented, these tests can be deleted.
Regarding this specific change,
i32is now a class that implements ourNumberInterfaceinterface, allowing implementation of some of the specific features we need to match WebAssembly integer maths. With this PR we now support signed / unsigned division, multiplication uses Long to ensure we don't exceed JS numeric precision, and various runtime exceptions are being thrown (as per spec)