[webkit-reviews] review granted: [Bug 166982] WebAssembly: implement Module imports/exports : [Attachment 305568] patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 166982: WebAssembly: implement Module imports/exports
https://bugs.webkit.org/show_bug.cgi?id=166982

Attachment 305568: patch

https://bugs.webkit.org/attachment.cgi?id=305568&action=review




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 305568
  --> https://bugs.webkit.org/attachment.cgi?id=305568
patch

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

r=me with comments

> JSTests/wasm/js-api/Module.exports.js:34
> +    assert.eq(m.exports[0].name, "func");
> +    assert.eq(m.exports[0].kind, "function");

Please add a test that repeated calls produce a new array.

> JSTests/wasm/js-api/Module.imports.js:23
> +    assert.eq(m.imports.length, 4);

Please add a test that repeated calls produce a new array.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:102
> +    if (!module)
> +	   throwException(exec, throwScope, createTypeError(exec,
ASCIILiteral("WebAssembly.Module.prototype.imports called with non
WebAssembly.Module |this| value")));
> +    RETURN_IF_EXCEPTION(throwScope, { });

This is weird style. You know when the exception happens, since you throw it.
So just return then, and don't have the RETURN_IF_EXCEPTION

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:108
> +    for (const Wasm::Import& i : imports) {

style: Call this "import" instead of "i" please.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:113
> +	   obj->putDirect(vm, Identifier::fromString(exec, "module"),
jsString(exec, i.module.string()));
> +	   obj->putDirect(vm, Identifier::fromString(exec, "name"),
jsString(exec, i.field.string()));
> +	   obj->putDirect(vm, Identifier::fromString(exec, "kind"),
jsString(exec, String(makeString(i.kind))));

I'd hoist the Identifier creation out of this loop just for perf sanity. I
don't think LLVM will figure out this won't change.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:130
> +    if (!module)
> +	   throwException(exec, throwScope, createTypeError(exec,
ASCIILiteral("WebAssembly.Module.prototype.exports called with non
WebAssembly.Module |this| value")));
> +    RETURN_IF_EXCEPTION(throwScope, { });

Ditto about exception throwing style.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:136
> +    for (const Wasm::Export& e : exports) {

Style: Please name this "export" instead of "e".

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:140
> +	   obj->putDirect(vm, Identifier::fromString(exec, "name"),
jsString(exec, e.field.string()));
> +	   obj->putDirect(vm, Identifier::fromString(exec, "kind"),
jsString(exec, String(makeString(e.kind))));

Ditto about moving Identifier creation out of the loop.