Bug 192355 – [WHLSL] Add a handwritten parser

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[WHLSL] Add a handwritten parser

[Myles C. Maxfield]

Created attachment 356468 [details]
WIP

[Myles C. Maxfield]

Created attachment 356569 [details]
WIP

[Myles C. Maxfield]

Created attachment 356570 [details]
WIP

[Myles C. Maxfield]

Created attachment 356585 [details]
WIP

[Myles C. Maxfield]

Created attachment 356695 [details]
WIP

[Myles C. Maxfield]

Created attachment 356778 [details]
WIP

[Myles C. Maxfield]

Created attachment 356863 [details]
WIP

[Myles C. Maxfield]

Created attachment 356947 [details]
WIP

[Myles C. Maxfield]

Created attachment 357031 [details]
WIP

[Myles C. Maxfield]

Created attachment 357035 [details]
WIP

[Myles C. Maxfield]

Created attachment 357038 [details]
WIP

[Myles C. Maxfield]

Created attachment 357081 [details]
WIP

[Myles C. Maxfield]

Created attachment 357103 [details]
WIP

[Myles C. Maxfield]

Created attachment 357199 [details]
WIP

[Myles C. Maxfield]

Created attachment 357207 [details]
WIP

[Myles C. Maxfield]

Created attachment 357225 [details]
First draft

[Myles C. Maxfield]

Created attachment 357245 [details]
Hooking it up

[Myles C. Maxfield]

1. Create the standard library at build time
2. Make sure the standard library parses

[Myles C. Maxfield]

Created attachment 357268 [details]
WIP

[Myles C. Maxfield]

Created attachment 357277 [details]
Standard Library

[Myles C. Maxfield]

Created attachment 357303 [details]
Starting to debug

[Myles C. Maxfield]

Created attachment 357304 [details]
Lexer needs some love

[Myles C. Maxfield]

Right now, we have a "sample" keyword, and there is a type in the standard library named "sampler". Because of how the lexer prefers keywords rather than Identifiers, "sampler" is getting split into a "sample" token and an "r" identifier.

[Myles C. Maxfield]

Created attachment 357361 [details]
WIP

[Myles C. Maxfield]

Created attachment 357410 [details]
Standard library parses

[Myles C. Maxfield]

Created attachment 357430 [details]
Finishing up move to Expected

[Myles C. Maxfield]

Created attachment 357458 [details]
Moved to Expected

[Myles C. Maxfield]

Created attachment 357520 [details]
Need to improve performance

[Myles C. Maxfield]

Created attachment 357602 [details]
Patch

[Myles C. Maxfield]

Created attachment 357749 [details]
Needs to build on Sierra

[Myles C. Maxfield]

Created attachment 357754 [details]
Limit the standard library

[Myles C. Maxfield]

Created attachment 357765 [details]
Limit the standard library

[Myles C. Maxfield]

Created attachment 357869 [details]
Compiles on Sierra

[Myles C. Maxfield]

Created attachment 357889 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 357889 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +               https://github.com/gpuweb/WHLSL/blob/master/Spec/WHLSL.g4.The parser uses Expected<> to return an appropraite Error string

2 typos

[Myles C. Maxfield]

Comment on attachment 357889 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +               https://github.com/gpuweb/WHLSL/blob/master/Spec/WHLSL.g4.The parser uses Expected<> to return an appropraite Error string
> 
> 2 typos

2 typos

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLASTConstantExpression.h:51
> +class ConstantExpression : public Variant<
> +    IntegerLiteral,
> +    UnsignedIntegerLiteral,
> +    FloatLiteral,
> +    NullLiteral,
> +    BooleanLiteral,
> +    ConstantExpressionEnumerationMemberReference
> +    > {

We can turn this back into a regular Variant as soon as we don't have to support Sierra.

[Myles C. Maxfield]

Created attachment 357929 [details]
Patch

[Saam Barati]

Comment on attachment 357929 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLASTIndexExpression.h:39
> +class IndexExpression : public PropertyAccessExpression {

Do these all really need to be in their own files? Seems a bit overkill to me

[EWS Watchlist]

Comment on attachment 357929 [details]
Patch

Attachment 357929 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10503879

New failing tests:
webgpu/simple-triangle-strip.html
webgpu/WHLSL/parse.html

[EWS Watchlist]

Created attachment 357932 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Tim Horton]

Comment on attachment 357929 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLASTBaseSemantic.h:1
> +/*

I have no skin in this game, but it seems like duplicating the namespace/directory hierarchy in the filename is a bit... unusual, and annoying. And why not go all the way and call it WebCoreModuleWebGPUWHLSLASTBaseSemantic.h?

[Myles C. Maxfield]

(In reply to Tim Horton from comment #41)
> Comment on attachment 357929 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357929&action=review
> 
> > Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLASTBaseSemantic.h:1
> > +/*
> 
> I have no skin in this game, but it seems like duplicating the
> namespace/directory hierarchy in the filename is a bit... unusual, and
> annoying. And why not go all the way and call it
> WebCoreModuleWebGPUWHLSLASTBaseSemantic.h?

The reason I did it is to avoid header include conflicts. All headers in WebCore get flattened to a single namespace, and I didn't want to take "Parser.h" or "IfStatement.h".

That being said, you're not the only person who has noticed this, so I guess I can at least change WHLSLASTIfStatement.h to WHLSLIfStatement.h

[Myles C. Maxfield]

Breaking this patch up

[Myles C. Maxfield]

Created attachment 357975 [details]
Patch

[Sam Weinig]

Comment on attachment 357975 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:120
> +auto Parser::fail(const String& message) -> Unexpected<Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:123
> +    return Unexpected<Error>(Error(String::format("Cannot lex: %s", message.utf8().data())));

We've been generally moving away from String::format where we can.  This could be replaced with makeString("Cannot lex: ", string). This is true of the other uses of String::format below.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:126
> +auto Parser::peek() -> Expected<Lexer::Token, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:131
> +    return fail(String("Cannot consume token", String::ConstructFromLiteral));

You should be able to use the "Cannot consume token"_s here (or if not, we should fix that). This is true for the other uses of the String::ConstructFromLiteral constructor below.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:154
> +auto Parser::consumeType(Lexer::Token::Type type) -> Expected<Lexer::Token, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:163
> +auto Parser::consumeTypes(Vector<Lexer::Token::Type> types) -> Expected<Lexer::Token, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:207
> +    for (unsigned i = 0; i < text.length(); ++i) {

We've mostly been trying to move toward using explicit StringView iterator types for iterating through text (e.g. codeUnits()/codePoints()/graphemeClusters()). When not doing that, it is best to explain why.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:227
> +    ASSERT(text.substring(text.length() - 1) == "u");

You could use endsWith here.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:230
> +    for (unsigned i = 0; i < text.length(); ++i) {

We've mostly been trying to move toward using explicit StringView iterator types for iterating through text (e.g. codeUnits()/codePoints()/graphemeClusters()). When not doing that, it is best to explain why.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:240
> +static Expected<float, Parser::Error> floatLiteralToFloat(StringView text)

You should see if you can use WTF::parseDouble() and the double_conversion code to implement this.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:268
> +auto Parser::consumeIntegralLiteral() -> Expected<Variant<int, unsigned>, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:292
> +auto Parser::consumeNonNegativeIntegralLiteral() -> Expected<unsigned, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:324
> +auto Parser::parseConstantExpression() -> Expected<AST::ConstantExpression, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:377
> +auto Parser::parseTypeArgument() -> Expected<AST::TypeArgument, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:389
> +auto Parser::parseTypeArguments() -> Expected<AST::TypeArguments, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:428
> +auto Parser::parseTypeSuffixAbbreviated() -> Expected<TypeSuffixAbbreviated, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:444
> +auto Parser::parseTypeSuffixNonAbbreviated() -> Expected<TypeSuffixNonAbbreviated, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:463
> +auto Parser::parseAddressSpaceType() -> Expected<std::unique_ptr<AST::Type>, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:506
> +auto Parser::parseNonAddressSpaceType() -> Expected<std::unique_ptr<AST::Type>, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:545
> +auto Parser::parseType() -> Expected<std::unique_ptr<AST::Type>, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:561
> +auto Parser::parseTypeDefinition() -> Expected<AST::TypeDefinition, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:580
> +auto Parser::parseBuiltInSemantic() -> Expected<AST::BuiltInSemantic, Error> {

{ should be on the next line.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:638
> +auto Parser::parseResourceSemantic() -> Expected<AST::ResourceSemantic, Error> {

{ should be on the next line.

[Myles C. Maxfield]

Created attachment 358866 [details]
WIP

[Myles C. Maxfield]

(In reply to Sam Weinig from comment #45)
> Comment on attachment 357975 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357975&action=review
> 
> > Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:126
> > +auto Parser::peek() -> Expected<Lexer::Token, Error> {
> 
> { should be on the next line.

When I do this, check-webkit-style says "This { should be at the end of the previous line  [whitespace/braces] [4]"

> 
> > Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:131
> > +    return fail(String("Cannot consume token", String::ConstructFromLiteral));
> 
> You should be able to use the "Cannot consume token"_s here (or if not, we
> should fix that). This is true for the other uses of the
> String::ConstructFromLiteral constructor below.

Yep. _s is for StringView. _str is for strings.

[Myles C. Maxfield]

Created attachment 358880 [details]
Patch

[Myles C. Maxfield]

Created attachment 358902 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 358902 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:915
> +    Optional<UniqueRef<AST::UnnamedType>> type;
> +    if (tryType(Lexer::Token::Type::Colon)) {
> +        auto parsedType = parseType();
> +        if (!parsedType)
> +            return Unexpected<Error>(parsedType.error());
> +        type = WTFMove(*parsedType);
> +    }

"type" needs to be set to "int" if it isn't explicitly present.

[Sam Weinig]

(In reply to Myles C. Maxfield from comment #47)
> (In reply to Sam Weinig from comment #45)
> > Comment on attachment 357975 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=357975&action=review
> > 
> > > Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:126
> > > +auto Parser::peek() -> Expected<Lexer::Token, Error> {
> > 
> > { should be on the next line.
> 
> When I do this, check-webkit-style says "This { should be at the end of the
> previous line  [whitespace/braces] [4]"
> 

check-webkit-style is wrong in this case :).

[Myles C. Maxfield]

Created attachment 359250 [details]
Patch

[EWS Watchlist]

Attachment 359250 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:157:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:186:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:196:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:288:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:313:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:346:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:400:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:413:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:453:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:470:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:508:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:576:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:619:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:636:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:656:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:715:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:777:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:785:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:806:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:857:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:892:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:924:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:975:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:991:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1016:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1053:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1080:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1106:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1135:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1150:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1210:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1239:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1265:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1288:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1315:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1345:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1381:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1421:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1456:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1511:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1536:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1565:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1602:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1629:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1807:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1838:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1861:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1894:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2039:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2064:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2096:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2175:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2198:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2253:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2325:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2364:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2403:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2450:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2520:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2561:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:2594:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 61 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 359250 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLDotExpression.h:63
> -        return String::format("operator.%s=", m_fieldName.utf8().data());
> +        return makeString("operator.%s=", m_fieldName);

It looks like you forgot to remove the %s here and below.

[Dean Jackson]

Comment on attachment 359250 [details]
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?

[Dean Jackson]

I think it would be possible to write some tests for this. Depends how long before the full functionality is in place for the real tests.

[Myles C. Maxfield]

Committed r240092: <https://trac.webkit.org/changeset/240092>

[Radar WebKit Bug Importer]

<rdar://problem/47333841>

[Alex Christensen]

Comment on attachment 359250 [details]
Patch

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

Sorry I haven't read this whole thing.  Does this parser use recursion, or does it use a stack on the heap?  The latter would be resistant to deeply nested things in programs causing stack overflows in the parser.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:175
> +Optional<Lexer::Token> Parser::tryTypes(Vector<Lexer::Token::Type> types)

This could be a const Vector& to avoid unnecessary copies.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:195
> +auto Parser::consumeTypes(Vector<Lexer::Token::Type> types) -> Expected<Lexer::Token, Error>

ditto

[Keith Miller]

(In reply to Alex Christensen from comment #59)
> Comment on attachment 359250 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359250&action=review
> 
> Sorry I haven't read this whole thing.  Does this parser use recursion, or
> does it use a stack on the heap?  The latter would be resistant to deeply
> nested things in programs causing stack overflows in the parser.

To be fair, the JS parser uses recursion and I don't think stack overflows have been too much of a problem.

[Said Abou-Hallawa]

Comment on attachment 359250 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1644
> +    {
> +        auto ifStatement = backtrackingScope<Expected<AST::IfStatement, Error>>([&]() {
> +            return parseIfStatement();
> +        });
> +        if (ifStatement)
> +            return { makeUniqueRef<AST::IfStatement>(WTFMove(*ifStatement)) };
> +    }

Parser::parseStatement() has many blocks similar to this one. What is this block supposed to do? From looking at the code I think this is what is going to happen:
-- parseIfStatement() tries to match an if-statement from the Lexer.
-- If it matches it will return IfStatement object which this block will return. Otherwise it will return an error in ifStatement.error(). And the Lexer state will be stored back to its original state.

The questions I have are:
-- Where the error that parseIfStatement() generates is used? I think it is calculated but it is just ignored.
-- Why do we have to go through all the possible parsing blocks one after another searching for the right matcher and back tracking the Lexer every time we fail to find a matcher? Can't we write this function like this:

switch (m_lexer.consumeToken()) {
case Lexer::Token::Type::If:
    return parseIfStatement();

case Lexer::Token::Type::Switch:
    return parseSwitchStatement();
}

-- If an if-statement is written incorrectly Parser::parseStatement() will still have to go through all the blocks before it is rejected by the last block. Can't we make an early return if a syntax error happens? For example parseIfStatement() will return Expected<UniqueRef<AST::Statement>, Error> instead.
-- What is the plan for error recovery? How are you going to recover from a syntax error such that multiple errors can be reported at once.