[webkit-reviews] review granted: [Bug 204843] [JSC] Improve Wasm binary test coverage : [Attachment 414421] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 204843: [JSC] Improve Wasm binary test coverage
https://bugs.webkit.org/show_bug.cgi?id=204843

Attachment 414421: Patch

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 414421
  --> https://bugs.webkit.org/attachment.cgi?id=414421
Patch

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

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:185
> +    WASM_PARSER_FAIL_IF(flags != 0x0 && flags != 0x1, "resiable limits flag
should be 0x00 or 0x01 but ", flags);

Typo here in the word resizable. Also, I don’t think “should be 0x00 or 0x01
but 5” makes sense so maybe add the word “is” and also seems like we’d want to
format as 2-digit hex with a leading “0x” if that was easy, or just call the
espected values 0 and 1.

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:511
> +    WASM_PARSER_FAIL_IF(mutability != 0x0 && mutability != 0x1, "invalid
Global's mutability ", mutability);

Might want a colon here to make it clear that the trailing number is the
mutability, not just a stray number at the end of a line.

> Source/WTF/wtf/LEBDecoder.h:78
> +    static_assert(!std::is_unsigned_v<T>);

Consider using is_signed?

> Source/WTF/wtf/LEBDecoder.h:93
> +		       // This is not sign-extended, positive number. Then,
remaining bits should be (lastByteMask<T>() >> 1).

Grammar mistake here. Should be “... is a non-sign-extended, positive number.
Thus, the remaining ...”

> Source/WTF/wtf/LEBDecoder.h:94
> +		       // For example, in int32_t case, the last byte should be
less than, 0b00000111, since 7 * 4 + 3 = 31.

Extraneous comma here after “less than”.

> Tools/TestWebKitAPI/Tests/WTF/LEBDecoder.cpp:40
> +	   out << static_cast<unsigned>(v) << ", ";

I assume we want two-digit hex values here and could accomplish that with
std::setfill and std::setw. Also, why is the cast to unsigned valuable?