[webkit-reviews] review granted: [Bug 192355] [WHLSL] Add a handwritten parser : [Attachment 359250] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 192355: [WHLSL] Add a handwritten parser
https://bugs.webkit.org/show_bug.cgi?id=192355

Attachment 359250: Patch

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




--- Comment #55 from Dean Jackson <dino at apple.com> ---
Comment on attachment 359250
  --> https://bugs.webkit.org/attachment.cgi?id=359250
Patch

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

I am not the right person to review this patch, but here goes.

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLDotExpression.h:63
>> +	    return makeString("operator.%s=", m_fieldName);
> 
> It looks like you forgot to remove the %s here and below.

This should be makeString("operator.", m_fieldName, "=");

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLDotExpression.h:68
> +	   return makeString("operator&.%s", m_fieldName);

This should be makeString("operator&.", m_fieldName);

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:143
> +	   WTFLogAlways("%s", error->error.utf8().data());

I think this should be LOG(WebGPU,..)

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:234
> +    if (text.startsWith("-"_str)) {
> +	   negate = true;
> +	   text = text.substring(1);
> +    }
> +    int base = 10;
> +    if (text.startsWith("0x"_str)) {

Are Strings better than using _s here?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:248
> +	   static_assert(std::numeric_limits<long long int>::min() <
std::numeric_limits<int>::min(), "long long needs to be bigger than an int");

When would this ever fail?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:308
> +	   return Unexpected<Error>(Error("Something really bad
happened"_str));

Maybe be a bit more descriptive here.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:502
> +	   return Unexpected<Error>(Error("Something really bad
happened"_str));

Ditto. And for the others.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:940
> +	   return { makeUniqueRef<AST::TypeReference>(Lexer::Token(*origin),
"int"_str, AST::TypeArguments()) };

Not uint?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1168
> +	   auto type = consumeTypes({ Lexer::Token::Type::Vertex,
Lexer::Token::Type::Fragment });

What about compute?