[WHLSL] Add a handwritten parser
Created attachment 356468 [details] WIP
Created attachment 356569 [details] WIP
Created attachment 356570 [details] WIP
Created attachment 356585 [details] WIP
Created attachment 356695 [details] WIP
Created attachment 356778 [details] WIP
Created attachment 356863 [details] WIP
Created attachment 356947 [details] WIP
Created attachment 357031 [details] WIP
Created attachment 357035 [details] WIP
Created attachment 357038 [details] WIP
Created attachment 357081 [details] WIP
Created attachment 357103 [details] WIP
Created attachment 357199 [details] WIP
Created attachment 357207 [details] WIP
Created attachment 357225 [details] First draft
Created attachment 357245 [details] Hooking it up
1. Create the standard library at build time 2. Make sure the standard library parses
Created attachment 357268 [details] WIP
Created attachment 357277 [details] Standard Library
Created attachment 357303 [details] Starting to debug
Created attachment 357304 [details] Lexer needs some love
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.
Created attachment 357361 [details] WIP
Created attachment 357410 [details] Standard library parses
Created attachment 357430 [details] Finishing up move to Expected
Created attachment 357458 [details] Moved to Expected
Created attachment 357520 [details] Need to improve performance
Created attachment 357602 [details] Patch
Created attachment 357749 [details] Needs to build on Sierra
Created attachment 357754 [details] Limit the standard library
Created attachment 357765 [details] Limit the standard library
Created attachment 357869 [details] Compiles on Sierra
Created attachment 357889 [details] Patch
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
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.
Created attachment 357929 [details] Patch
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
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
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
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?
(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
Breaking this patch up
Created attachment 357975 [details] Patch
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.
Created attachment 358866 [details] WIP
(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.
Created attachment 358880 [details] Patch
Created attachment 358902 [details] Patch
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.
(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 :).
Created attachment 359250 [details] Patch
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.
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.
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?
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.
Committed r240092: <https://trac.webkit.org/changeset/240092>
<rdar://problem/47333841>
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
(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.
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.