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

[bugzilla-daemon at webkit.org]

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

--- Comment #19 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
Thanks you for the review.

(In reply to comment #16)
> > Source/JavaScriptCore/wasm/JSWASMModule.h:57
> > +    Vector<uint32_t>& i32Constants() { return m_i32Constants; }
> 
> Nit: These functions can all be marked as "const".

They cannot be marked as const, because I modify them.

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

Done. Changed it to "result" to be consistent with the name I use in WASMReader.

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

Done. Also added a check for numberOfSignatures in WASMModuleParser::parseFunctionImportSection() for safety.

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

Done. Changed it to TODO and added a link to https://bugs.webkit.org/show_bug.cgi?id=147738

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

Done. Added "static const uint32_t firstSevenBitsMask = 0x7f;". This will be used again when I add a signed version of compact ints. I agree that it would be easier to read if it's in binary, but I can't find 0b... used anywhere other than in comments. I'm not sure what our policy is on binary literals.

(In reply to comment #17)
> > Source/JavaScriptCore/wasm/WASMModuleParser.cpp:138
> > +        m_module->signatures().append(signature);
> 
> Same here, and a few places below..

Done.

-- 
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/935dd0cd/attachment.html>