[webkit-reviews] review requested: [Bug 171263] WebAssembly: support name section : [Attachment 309615] patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has asked  for review:
Bug 171263: WebAssembly: support name section
https://bugs.webkit.org/show_bug.cgi?id=171263

Attachment 309615: patch

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




--- Comment #8 from JF Bastien <jfbastien at apple.com> ---
Created attachment 309615

  --> https://bugs.webkit.org/attachment.cgi?id=309615&action=review

patch

> > JSTests/ChangeLog:10
> > +	     * wasm/function-tests/nameSection.wasm: Added.
> 
> Why is there a raw wasm file?

Two main reasons:

  1. I wanted to test exactly what Emscripten generates for name section (it
adds some more stuff than the names in the C++ source file).
  2. I don't think this is particularly useful to add to the builder.

In general it's suboptimal to check in binary files, but here I think it's the
right thing to do. The repro instructions are in the .js file (original C++
file and build command line).


> > Source/JavaScriptCore/wasm/WasmCallee.h:63
> > +	 IndexOrName m_indexOrName;
> 
> One downside of using IndexOrName here is that you have no way to get the
> index of the Callee back if that's what you want. Perhaps we don't need that
> but it also seems like if we have the index we can recover the name from the
> module but not the other way around.

Yeah that seems fine for now, but we can revisit later if we want both.


> > Source/JavaScriptCore/wasm/WasmFormat.h:244
> > +static bool isValidNameType(Int val)
> 
> shouldn't this be inline not static? I guess static might be ok since this
> is a template function but it still seems weird.

True, I updated here and the similar thing in this file.


> > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:607
> > +	 Name nameName = { 'n', 'a', 'm', 'e' };
> 
> name name name name!

I know right! I chuckled when I wrote this.


> > Source/JavaScriptCore/wasm/WasmName.h:35
> > +typedef Vector<LChar> Name;
> 
> nit: using Name = Vector<LChar>;

lol C++14. I always forget about "using" it.


> > Source/JavaScriptCore/wasm/WasmNameSectionParser.cpp:61
> > +		     WASM_PARSER_FAIL_IF(!parseVarUInt32(nameLen), "can't get
", function, "th function's name length for payload ", payloadNumber);
> > +		     WASM_PARSER_FAIL_IF(!consumeUTF8String(nameString,
nameLen), "can't get ", function, "th function's name of length ", nameLen, "
for payload ", payloadNumber);
> 
> Nit: why are these ths? I would phase this as "unable to read function ",
> function, "'s name, which has length of ", nameLen, " and payload",
> payloadNumber

Consistent with: https://bugs.webkit.org/show_bug.cgi?id=166688
Your phrasing is nicer though. Adopting here.