[webkit-reviews] review granted: [Bug 184689] [WebAssembly][Modules] Implement function import from wasm modules : [Attachment 338108] Patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 184689: [WebAssembly][Modules] Implement function import from wasm modules
https://bugs.webkit.org/show_bug.cgi?id=184689

Attachment 338108: Patch

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




--- Comment #5 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 338108
  --> https://bugs.webkit.org/attachment.cgi?id=338108
Patch

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

Wooh! Very cool :)
r=me

> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:58
> +	       specifier->importedName() ==
analyzer.vm().propertyNames->timesIdentifier,

It's nicer to have an enum class here, like:
  specifier->importedName() == analyzer.vm().propertyNames->timesIdentifier ?
IMPORT_ENTIRE_NAMESPACE : IMPORT_SINGLE_IDENTIFIER

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:203
> +	       false,

Ditto here for enum class.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:158
> +		   throwSyntaxError(exec, scope, makeString("Importing binding
name 'default' cannot be resolved by star export entries."));

This message confused me. How can it happen?

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:184
> +	       value = jsUndefined();

Isn't it a TDZ in this case? IIUC that's where you have a circular import, and
you're going to try using it in evaluate() ? That should fail wasm loading,
just like TDZ does for JS.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:230
> +	   }

Drop extra braces here and below.

> Tools/Scripts/run-jsc-stress-tests:1017
> +def runWebAssemblyDirect

Maybe it's better to rename runWebAssembly above? The one you're adding is
really the "run the simple thing", whereas above is "run with all these extra
imports". This could be a follow-up cleanup.