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

[bugzilla-daemon at webkit.org]

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

--- Comment #16 from Saam Barati <saambarati1 at gmail.com> ---
Comment on attachment 258288
  --> https://bugs.webkit.org/attachment.cgi?id=258288
Override JSCell::destroy()

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

Looks pretty good to me. This was a fly-by review. I'll look at it in more detail and look at the polyfill when I have more time.

> Source/JavaScriptCore/wasm/JSWASMModule.h:57
> +    Vector<uint32_t>& i32Constants() { return m_i32Constants; }

Nit: These functions can all be marked as "const".

> Source/JavaScriptCore/wasm/WASMModuleParser.cpp:38
> +#define READ_UINT32_OR_FAIL(x, errorMessage) do { if (!m_reader.readUInt32(x)) FAIL_WITH_MESSAGE(errorMessage); } while (0)

I think we should use better names than "x" here and below. Maybe like numConstants or inOutNumConstants?

> Source/JavaScriptCore/wasm/WASMModuleParser.cpp:108
> +        m_module->i32Constants().append(constant);

I think this (and all appends below) can be uncheckedAppend() since we're reserving their space up front.

> Source/JavaScriptCore/wasm/WASMModuleParser.cpp:257
> +    // TODO: Support any functions.

Nit: We tend to use FIXME instead of TODO
Also, is there a bug open for this? If so, let's include the link to it here.
If not, we should open one and link it here.

> Source/JavaScriptCore/wasm/WASMReader.cpp:81
> +bool WASMReader::readCompactUInt32(uint32_t& result)

I wonder if some of this would be easier to read if we wrote numbers in binary or gave then names.
Like: 
uint8_t firstSevenBits = 0x7f;
or
uint8_t byteValueMask = 0x7f;

Or maybe this is silly and we should just stick with hex.

-- 
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/20150806/e46d7755/attachment.html>