[webkit-reviews] review granted: [Bug 170478] WebAssembly: ModuleInformation should be a ref counted thing that can be shared across threads. : [Attachment 306222] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 170478: WebAssembly: ModuleInformation should be a ref counted thing that
can be shared across threads.
https://bugs.webkit.org/show_bug.cgi?id=170478

Attachment 306222: Patch

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




--- Comment #7 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 306222
  --> https://bugs.webkit.org/attachment.cgi?id=306222
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +	   is likely, not a problem because:

no comma

> Source/JavaScriptCore/wasm/WasmModuleInformation.h:31
> +#include <wtf/RefCounted.h>

I think you want ThreadSafeRefCounted here. Or I guess you can remove the
include since it looks like it's doing nothing that you need.

> Source/JavaScriptCore/wasm/WasmModuleInformation.h:47
> +	   ? importFunctionSignatureIndices[functionIndex]
> +	   : internalFunctionSignatureIndices[functionIndex -
importFunctionSignatureIndices.size()];

Please indent.

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:100
> +    return { };

Why this change?

> Source/JavaScriptCore/wasm/WasmParser.h:166
> +    memcpy(result.data(), source() + m_offset, stringLength);

nit: you can use string start here, right?

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:170
> +    Ref<Plan> plan = adoptRef(*new Plan(vm,
makeRef(const_cast<Wasm::ModuleInformation&>(module->moduleInformation())),
Plan::FullCompile, [promise, instance, module, entries] (Plan& p) {

why not just return non const instead of us lying about const-ness?