[webkit-reviews] review granted: [Bug 219943] [WASM-References] Add support for memory.copy, memory.init and data.drop : [Attachment 416402] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Dmitry <dbezhetskov at igalia.com>'s
request for review:
Bug 219943: [WASM-References] Add support for memory.copy, memory.init and
data.drop
https://bugs.webkit.org/show_bug.cgi?id=219943

Attachment 416402: Patch

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




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

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:25
> +

Nice. It is also nice if we have DataCount section information (like, what it
is and why it is needed).

> Source/JavaScriptCore/wasm/WasmFormat.cpp:37
> +Segment* Segment::create(Optional<I32InitExpr> offset, uint32_t sizeInBytes,
Kind kind)

Oh, while this is not the changed part in this patch, I think we should return
`Segment::Ptr` from this function instead of calling `Segment::adoptPtr()`
later.
Then, Segment's memory is managed well by unique_ptr.

> Source/JavaScriptCore/wasm/WasmFormat.cpp:52
>      return segment;

Let's return `return Segment::adoptPtr(segment)`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:759
> +	       Segment* segment = Segment::create(*initExpr, dataByteLength,
Segment::Kind::Active);

Let's receive this as `auto segment = Segment::create(*initExpr,
dataByteLength, Segment::Kind::Active);`.
And let's keep Segment* pointer by doing like, `Segment* segmentPtr =
segment.get();`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:761
> +	       m_info->data.uncheckedAppend(Segment::adoptPtr(segment));

Let's pass it with `WTFMove(segment)`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:765
> +		   segment->byte(dataByte) = byte;

Use segmentPtr to access since segment is already moved.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:768
> +	   }

We can insert `ASSERT(Options::useWebAssemblyReferences())`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:775
> +	       Segment* segment = Segment::create(WTF::nullopt, dataByteLength,
Segment::Kind::Passive);

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:777
> +	       m_info->data.uncheckedAppend(Segment::adoptPtr(segment));

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:781
> +		   segment->byte(dataByte) = byte;

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:788
> +	       WASM_PARSER_FAIL_IF(!Options::useWebAssemblyReferences(),
"references are not enabled");

Let's remove this line since if `!Options::useWebAssemblyReferences()`, then
execution never reaches here since we have `if
(!Options::useWebAssemblyReferences() || !dataFlag) {`.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:800
> +	       Segment* segment = Segment::create(*initExpr, dataByteLength,
Segment::Kind::Active);

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:802
> +	       m_info->data.uncheckedAppend(Segment::adoptPtr(segment));

Ditto.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:806
> +		   segment->byte(dataByte) = byte;

Ditto.