Bug 164611 – Move Wasm tests to JS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Move Wasm tests to JS

[Keith Miller]

Created attachment 294402 [details]
Patch

[Keith Miller]

Comment on attachment 294402 [details]
Patch

Shouldn't have been marked for review.

[WebKit Commit Bot]

Attachment 294402 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jsc.cpp:2448:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 294402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294402&action=review

This is pretty great!

> Source/JavaScriptCore/jsc.cpp:1163
> +        addFunction(vm, "testWasmModule", functionTestWasmModule, 0);

What does it test about a wasm module? I think the name could be improved.

> Source/JavaScriptCore/jsc.cpp:2377
> +        sscanf(bitwise_cast<const char*>(jsCast<JSString*>(value)->value(exec).characters8()), "%lld", &result);

Check return isn't EOF.

> Source/JavaScriptCore/jsc.cpp:2387
> +    if (typeString == "float")

Wouldn't "f32" be more consistent here?

> Source/JavaScriptCore/jsc.cpp:2390
> +    return value;

RELEASE_ASSERT(typeString == "f64");

> Source/JavaScriptCore/testWasm.cpp:464
> +        //       )I

?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:60
> +const bool verbose = true;

:-)

> JSTests/wasm/Builder.js:172
> +                                            }

Weird indent.

> JSTests/wasm/Builder.js:178
> +    assert.eq(imms.length, imm.length, `"${op}" expects ${imm.length} immediates, got ${imms.length}`);

What's "imm" here?

> JSTests/wasm/Builder.js:235
> +            print(nextBuilder)

Semicolon.

> JSTests/wasm/Builder.js:371
> +                        Memory: (initial, max) => s.data.push({ initial, max })

I think it would be better to spell out initial and max when used, instead of relying on order here. Maybe have Memory().InitialMaxPages(x, y).End() ? We'll also allow more than 1 in the future, so the API has to support more than one memory.

> JSTests/wasm/Builder_WebAssemblyBinary.js:76
> +            put(bin, "varuint32", memory.max ? 1 : 0);

Comment to explain what this is: // flags

> JSTests/wasm/LowLevelBinary.js:231
> +        print(this._buffered_bytes);

Nope!

[Keith Miller]

Created attachment 294537 [details]
Patch

[Keith Miller]

Created attachment 294541 [details]
Patch

[Geoffrey Garen]

Comment on attachment 294541 [details]
Patch

We want one test per file.

[Keith Miller]

Created attachment 294555 [details]
Patch

[Keith Miller]

Created attachment 294556 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 294556 [details]
Patch for landing

Clearing flags on attachment: 294556

Committed r208627: <http://trac.webkit.org/changeset/208627>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.