[webkit-reviews] review granted: [Bug 234116] [WebAssembly][Modules] Unify memory import handling code in both module loader and JS cases : [Attachment 448631] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Asumu Takikawa <asumu at igalia.com>'s
request for review:
Bug 234116: [WebAssembly][Modules] Unify memory import handling code in both
module loader and JS cases
https://bugs.webkit.org/show_bug.cgi?id=234116

Attachment 448631: Patch

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




--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 448631
  --> https://bugs.webkit.org/attachment.cgi?id=448631
Patch

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

While I’m not an expert on the Wasm subsystem, the change sounds logical, and
all the tests are passing.

> Source/JavaScriptCore/wasm/WasmModule.cpp:128
> +	   CalleeGroup* calleeGroup = m_calleeGroups[i].get();
> +	   // We should only try to copy the group here if it hasn't already
been created.
> +	   // If it exists but is not runnable, it should get compiled during
module evaluation.
> +	   if (calleeGroup)
> +	       continue;
>	   Ref<CalleeGroup> newBlock =
CalleeGroup::createFromExisting(static_cast<MemoryMode>(i), initialBlock);
>	   m_calleeGroups[i] = WTFMove(newBlock);

The local variables are making this code less readable. It should look more
like this.

    // We should only try to copy the group here if it hasn't already been
created.
    // If it exists but is not runnable, it should get compiled during module
evaluation.
    if (!m_calleeGroups[i])
	m_calleeGroups[i] =
CalleeGroup::createFromExisting(static_cast<MemoryMode>(i), initialBlock);

In fact, I would go even further:

    // We should only try to copy the group here if it hasn't already been
created.
    // If it exists but is not runnable, it should get compiled during module
evaluation.
    if (auto& group = m_calleeGroups[i]; !group)
	group = CalleeGroup::createFromExisting(static_cast<MemoryMode>(i),
initialBlock);

But some people might find that style subtle or confusing.