[Webkit-unassigned] [Bug 147393] Parse the entire WebAssembly modules

[bugzilla-daemon at webkit.org]

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

Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #257743|review?                     |review-
              Flags|                            |

--- Comment #3 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 257743
  --> https://bugs.webkit.org/attachment.cgi?id=257743
Patch

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

The direction here looks good, but this patch needs some work.

> Source/JavaScriptCore/wasm/JSWASMModule.h:90
> +enum class WASMType : uint8_t {
> +    I32,
> +    F32,
> +    F64
> +};
> +
> +enum class WASMReturnType : uint8_t {
> +    I32,
> +    F32,
> +    F64,
> +    Void
> +};
> +
> +enum class WASMExportFormat : uint8_t {
> +    Default,
> +    Record
> +};
> +
> +struct WASMSignature {
> +    WASMReturnType returnType;
> +    Vector<WASMType> arguments;
> +};
> +
> +struct WASMFunctionImport {
> +    String functionName;
> +};
> +
> +struct WASMFunctionImportSignature {
> +    uint32_t signatureIndex;
> +    uint32_t functionImportIndex;
> +};
> +
> +struct WASMFunctionDeclaration {
> +    uint32_t signatureIndex;
> +};
> +
> +struct WASMFunctionPointerTable {
> +    uint32_t signatureIndex;
> +    Vector<uint32_t> elements;
> +};

In general, we keep one file per class in WebKit. Since these structs are separate from JSWASMModule, they should belong somewhere else.

It would be tedious to use one file per class for each of these structs, so let's use a shared file for all of the structs that define the format of a WASM tree. How about WASMFormat.h, as you suggested previously?

> Source/JavaScriptCore/wasm/JSWASMModule.h:113
> +    friend class WASMModuleParser;

"friend" tends toward spaghetti code. We use classes to establish a separation of concerns, so that we can think about one class, and change it, without worrying about all the other code that uses it. "friend" erases that separation and makes our code more brittle.

Instead of friend, in this case I recommend providing accessors methods for these underlying vectors.

> Source/JavaScriptCore/wasm/WASMModuleParser.h:60
> +    JSWASMModule* m_module;

You need to do something to prevent m_module from being garbage collected.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150729/b7813dc2/attachment.html>