[webkit-reviews] review granted: [Bug 176285] WSL should support the bool type : [Attachment 319858] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 176285: WSL should support the bool type
https://bugs.webkit.org/show_bug.cgi?id=176285

Attachment 319858: Patch

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




--- Comment #10 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 319858
  --> https://bugs.webkit.org/attachment.cgi?id=319858
Patch

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

r=me. 

Would be good to clean up the way the parser and leader interact. It’s easier
in the long run if the lexer doesn’t know too much about keywords. Please also
fix the WTypeError issue and the origin issue. The rest (like naming) is
optional.

> Tools/WebGPUShadingLanguageRI/Checker.js:229
> +	       throw new Error("Trying to negate something that isn't a bool: "
+ node.value);

This should be a WTypeError since this case can be reached with bad input. (The
Error above is fine because that will only happen with a bug in our code.)

> Tools/WebGPUShadingLanguageRI/Intrinsics.js:209
> +	       throw new WTypeError(thing.toString(), "Unrecognized intrinsic:
" + thing);

The first arg should be the origin string since that’s what we do everywhere
else. Can we do that here?

> Tools/WebGPUShadingLanguageRI/Lexer.js:106
> +	       if (isAddressSpace(RegExp.lastMatch))

Why can’t this get handled as a keyword?  If it’s not a keyword, why are
address spaces still in the keyword regezp?

> Tools/WebGPUShadingLanguageRI/Lexer.js:108
> +	       if (["true", "false"].includes(RegExp.lastMatch))

Ditto. 

Compilers usually recognize all such things as keywords. It’s up to the parser
to decide how to categorize keywords.

> Tools/WebGPUShadingLanguageRI/LogicalNegationExpression.js:27
> +class LogicalNegationExpression extends Expression {

This is usually called LogicalNot

> Tools/WebGPUShadingLanguageRI/Parse.js:188
> +	   if (token = tryConsumeKind("boolLiteral"))

You could have said tryConsume(“true”, “false”)