[webkit-reviews] review denied: [Bug 218744] [WASM-References] Support imm for ref.null : [Attachment 413779] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied  review:
Bug 218744: [WASM-References] Support imm for ref.null
https://bugs.webkit.org/show_bug.cgi?id=218744

Attachment 413779: Patch

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




--- Comment #4 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 413779
  --> https://bugs.webkit.org/attachment.cgi?id=413779
Patch

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

Nice, the design of the patch looks good!

> Source/JavaScriptCore/ChangeLog:6
> +	   Reviewed by NOBODY (OOPS!).

Can you describe the explanation of changes in ChangeLog?
And can you also add a link to the spec of this change in the ChangeLog?

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:497
> +	   WASM_PARSER_FAIL_IF(!parseValueType(typeOfNull), "can't get
ref.null's type");

This is checking whether the value is ValueType. I think we need to validate
whether this is RefType.
Can you add a test to ensure that module validation fails if the input is not a
retype?

> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:478
> +	   WASM_PARSER_FAIL_IF(!parseValueType(typeOfNull), "can't get
ref.null's type");

Ditto.

> Source/JavaScriptCore/wasm/wasm.json:62
> +	   "ref.null":		  { "category": "special",    "value": 208,
"return": ["externref", "funcref"],	    "parameter": [],		       
	 "immediate": [{"name": "reftype",	  "type": "elem_type"}],       
				     "description": "a constant null reference"
},

Ditto.

> JSTests/wasm/Builder.js:311
> +	       assert.truthy(WASM.isValidElemType(imms[idx]), `Invalid elem
type on ${op}: "${imms[idx]}"`);

Let's rename ElemType to RefType.

> JSTests/wasm/LowLevelBinary.js:206
> +    elem_type(v) {
> +	   if (!WASM.isValidElemType(v))

Ditto. ref_type and RefType.

> JSTests/wasm/WASM.js:45
> +const _elemTypeSet = new Set(description.elem_type);
> +export const isValidElemType = v => _elemTypeSet.has(v);

Ditto.

> JSTests/wasm/wasm.json:62
> +	   "ref.null":		  { "category": "special",    "value": 208,
"return": ["externref", "funcref"],	    "parameter": [],		       
	 "immediate": [{"name": "reftype",	  "type": "elem_type"}],       
				     "description": "a constant null reference"
},

Can you rename this "elem_type" to "ref_type"?
https://webassembly.github.io/reference-types/core/binary/instructions.html#ref
erence-instructions