[webkit-reviews] review granted: [Bug 219192] [WASM-References] Add support for active mods in element section : [Attachment 415097] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Dmitry <dbezhetskov at igalia.com>'s
request for review:
Bug 219192: [WASM-References] Add support for active mods in element section
https://bugs.webkit.org/show_bug.cgi?id=219192

Attachment 415097: Patch

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




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

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

r=me with suggestions.

> Source/JavaScriptCore/wasm/WasmFormat.h:233
> +    constexpr static uint32_t NullFuncIndex = UINT32_MAX;

Let's define it as `nullFuncIndex`, which is WebKit's naming convention for
constants.

> Source/JavaScriptCore/wasm/WasmFormat.h:235
> +    enum class Kind {

Let's make it `enum class Kind : uint8_t`.

> Source/JavaScriptCore/wasm/WasmFormat.h:248
> +    bool active() const { return kind == Kind::Active; }

isActive() would be better.

> Source/JavaScriptCore/wasm/WasmFormat.h:253
> +    TableElementType elemType;

Let's name it elementType.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:417
> +	       // https://bugs.webkit.org/show_bug.cgi?id=219297

Add FIXME with comment about what is missing.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:428
> +	       uint8_t elemKind;

=> elementKind.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:450
> +	       // https://bugs.webkit.org/show_bug.cgi?id=219385

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:480
> +	       // https://bugs.webkit.org/show_bug.cgi?id=219297

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:516
> +	       // https://bugs.webkit.org/show_bug.cgi?id=219385

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.h:75
> +    PartialResult WARN_UNUSED_RETURN parseElemKind(uint8_t& elemKind);
> +    PartialResult WARN_UNUSED_RETURN
parseIndexCountForElemSection(uint32_t&, const unsigned);
> +    PartialResult WARN_UNUSED_RETURN
parseFuncIdxFromRefExpForElemSection(uint32_t&, const unsigned, const
unsigned);
> +    PartialResult WARN_UNUSED_RETURN parseFuncIdxForElemSection(uint32_t&,
const unsigned, const unsigned);

Let's rename Elem to Element. WebKit prefers non-abbreviated names :)
And since we have parseElement, using "Element" is good for consistency.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:558
> +	       if (element.active()) {

if (!element.isActive())
    continue;

can be cleaner.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:619
>	       // for the import.

Let's move `uint32_t functionIndex = element.functionIndices[i];` to the
prologue of this loop body and use `functionIndex` instead of
`element.functionIndices[i]`.