[webkit-reviews] review denied: [Bug 198157] [WASM-References] Add support for Funcref in parameters and return types : [Attachment 372095] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 198157: [WASM-References] Add support for Funcref in parameters and return
types
https://bugs.webkit.org/show_bug.cgi?id=198157

Attachment 372095: Patch

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




--- Comment #5 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 372095
  --> https://bugs.webkit.org/attachment.cgi?id=372095
Patch

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

I found GC bug, so r-, but the code looks really nice!

> Source/JavaScriptCore/wasm/WasmInstance.cpp:98
> +void setWasmTableElement(Wasm::Instance* instance, uint32_t idx, uint64_t
encValue)

Is encValue always encoded JSValue? If so, let's use EncodedJSValue type
instead. And the name "encodedValue" is better.
I also like "index" instead of "idx".

> Source/JavaScriptCore/wasm/WasmInstance.cpp:123
> +    JSValue val = instance->getFunctionWrapper(index);

val => value.

> Source/JavaScriptCore/wasm/WasmInstance.h:167
> +    HashMap<uint32_t, WriteBarrier<Unknown>, IntHash<uint32_t>,
WTF::UnsignedWithZeroKeyHashTraits<uint32_t>> m_functionWrappers;

I think visitChildren handling for m_functionWrappers is missing.

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:127
> +	   emitWasmToJSException(jit, GPRInfo::argumentGPR2,
argumentsIncludeI64 ? ExceptionType::I64ArgumentType :
ExceptionType::I64ReturnType);

I like to see the word "Throw" for such a function. So,
"emitThrowWasmToJSException" would be nice.