[Webkit-unassigned] [Bug 234116] [WebAssembly][Modules] Unify memory import handling code in both module loader and JS cases

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=234116

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com
 Attachment #448631|review?                     |review+
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220109/cb2b3c32/attachment.htm>