[webkit-reviews] review denied: [Bug 189502] [WHLSL] Improve test suite type safety : [Attachment 349399] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has denied Thomas Denney
<tdenney at apple.com>'s request for review:
Bug 189502: [WHLSL] Improve test suite type safety
https://bugs.webkit.org/show_bug.cgi?id=189502

Attachment 349399: Patch

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




--- Comment #2 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 349399
  --> https://bugs.webkit.org/attachment.cgi?id=349399
Patch

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

> Tools/WebGPUShadingLanguageRI/Casts.js:45
> +function castToBool(value)
> +{
> +    return !!value;
> +}

Can we put this with the rest of the casts?

> Tools/WebGPUShadingLanguageRI/Test.js:83
> -    return TypedValue.box(program.intrinsics.int, value);
> +    return TypedValue.box(program.intrinsics.int, castToInt(value));
>  }
>  
>  function makeUint(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.uint, value);
> +    return TypedValue.box(program.intrinsics.uint, castToUint(value));
>  }
>  
>  function makeUchar(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.uchar, value);
> +    return TypedValue.box(program.intrinsics.uchar, castToUchar(value));
>  }
>  
>  function makeBool(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.bool, value);
> +    return TypedValue.box(program.intrinsics.bool, castToBool(value));
>  }
>  
>  function makeFloat(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.float, value);
> +    return TypedValue.box(program.intrinsics.float, castToFloat(value));
>  }
>  
>  function makeHalf(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.half, value);
> +    return TypedValue.box(program.intrinsics.half, castToHalf(value));

perhaps instead of silently casting, we should throw if the input thing isn't
already the right type. The caller should know if the only reason the test is
passing is that the type system is interfering

> Tools/WebGPUShadingLanguageRI/Test.js:2702
> -    checkUchar(program, callFunction(program, "foo", [makeUchar(program,
65535), makeUint(program, 2)]), 255);
> -    checkUchar(program, callFunction(program, "foo", [makeUchar(program,
-1), makeUint(program, 5)]), 255);
> +    checkUchar(program, callFunction(program, "foo", [makeUchar(program,
65535), makeUint(program, 2)]), 63);
> +    checkUchar(program, callFunction(program, "foo", [makeUchar(program,
-1), makeUint(program, 5)]), 7);

What? Why is this necessary?