RESOLVED FIXED 192355
[WHLSL] Add a handwritten parser
https://bugs.webkit.org/show_bug.cgi?id=192355
Summary [WHLSL] Add a handwritten parser
Myles C. Maxfield
Reported 2018-12-03 22:15:46 PST
[WHLSL] Add a handwritten parser
Attachments
WIP (77.64 KB, patch)
2018-12-03 22:16 PST, Myles C. Maxfield
no flags
WIP (78.14 KB, patch)
2018-12-04 18:40 PST, Myles C. Maxfield
no flags
WIP (86.42 KB, patch)
2018-12-04 19:31 PST, Myles C. Maxfield
no flags
WIP (93.11 KB, patch)
2018-12-04 23:24 PST, Myles C. Maxfield
no flags
WIP (99.49 KB, patch)
2018-12-05 19:09 PST, Myles C. Maxfield
no flags
WIP (108.98 KB, patch)
2018-12-06 19:42 PST, Myles C. Maxfield
no flags
WIP (134.77 KB, patch)
2018-12-07 22:07 PST, Myles C. Maxfield
no flags
WIP (146.82 KB, patch)
2018-12-09 23:36 PST, Myles C. Maxfield
no flags
WIP (180.15 KB, patch)
2018-12-10 19:34 PST, Myles C. Maxfield
no flags
WIP (198.39 KB, patch)
2018-12-10 22:39 PST, Myles C. Maxfield
no flags
WIP (198.44 KB, patch)
2018-12-10 23:30 PST, Myles C. Maxfield
no flags
WIP (235.12 KB, patch)
2018-12-11 15:54 PST, Myles C. Maxfield
no flags
WIP (252.46 KB, patch)
2018-12-11 22:13 PST, Myles C. Maxfield
no flags
WIP (267.56 KB, patch)
2018-12-12 17:35 PST, Myles C. Maxfield
no flags
WIP (301.90 KB, patch)
2018-12-12 19:46 PST, Myles C. Maxfield
no flags
First draft (334.37 KB, patch)
2018-12-13 04:37 PST, Myles C. Maxfield
no flags
Hooking it up (4.76 MB, patch)
2018-12-13 12:34 PST, Myles C. Maxfield
no flags
WIP (4.93 MB, patch)
2018-12-13 15:56 PST, Myles C. Maxfield
no flags
Standard Library (4.05 MB, patch)
2018-12-13 17:51 PST, Myles C. Maxfield
no flags
Starting to debug (4.05 MB, patch)
2018-12-13 23:00 PST, Myles C. Maxfield
no flags
Lexer needs some love (4.05 MB, patch)
2018-12-13 23:54 PST, Myles C. Maxfield
no flags
WIP (4.04 MB, patch)
2018-12-14 17:34 PST, Myles C. Maxfield
no flags
Standard library parses (4.06 MB, patch)
2018-12-15 16:50 PST, Myles C. Maxfield
no flags
Finishing up move to Expected (4.08 MB, patch)
2018-12-17 01:56 PST, Myles C. Maxfield
no flags
Moved to Expected (4.08 MB, patch)
2018-12-17 11:15 PST, Myles C. Maxfield
no flags
Need to improve performance (4.08 MB, patch)
2018-12-17 19:39 PST, Myles C. Maxfield
no flags
Patch (4.08 MB, patch)
2018-12-18 13:21 PST, Myles C. Maxfield
no flags
Needs to build on Sierra (4.05 MB, patch)
2018-12-19 16:20 PST, Myles C. Maxfield
no flags
Limit the standard library (359.28 KB, patch)
2018-12-19 16:44 PST, Myles C. Maxfield
no flags
Limit the standard library (359.32 KB, patch)
2018-12-19 17:21 PST, Myles C. Maxfield
no flags
Compiles on Sierra (356.44 KB, text/plain)
2018-12-20 14:11 PST, Myles C. Maxfield
no flags
Patch (361.53 KB, patch)
2018-12-20 16:01 PST, Myles C. Maxfield
no flags
Patch (361.75 KB, patch)
2018-12-20 23:00 PST, Myles C. Maxfield
ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.10 MB, application/zip)
2018-12-21 00:15 PST, EWS Watchlist
no flags
Patch (115.77 KB, patch)
2018-12-21 13:41 PST, Myles C. Maxfield
no flags
WIP (117.97 KB, patch)
2019-01-10 19:58 PST, Myles C. Maxfield
no flags
Patch (121.38 KB, patch)
2019-01-10 23:26 PST, Myles C. Maxfield
no flags
Patch (120.88 KB, patch)
2019-01-11 09:35 PST, Myles C. Maxfield
no flags
Patch (122.00 KB, patch)
2019-01-15 20:08 PST, Myles C. Maxfield
dino: review+
Myles C. Maxfield
Comment 1 2018-12-03 22:16:11 PST
Myles C. Maxfield
Comment 2 2018-12-04 18:40:25 PST
Myles C. Maxfield
Comment 3 2018-12-04 19:31:30 PST
Myles C. Maxfield
Comment 4 2018-12-04 23:24:30 PST
Myles C. Maxfield
Comment 5 2018-12-05 19:09:42 PST
Myles C. Maxfield
Comment 6 2018-12-06 19:42:12 PST
Myles C. Maxfield
Comment 7 2018-12-07 22:07:19 PST
Myles C. Maxfield
Comment 8 2018-12-09 23:36:20 PST
Myles C. Maxfield
Comment 9 2018-12-10 19:34:40 PST
Myles C. Maxfield
Comment 10 2018-12-10 22:39:27 PST
Myles C. Maxfield
Comment 11 2018-12-10 23:30:25 PST
Myles C. Maxfield
Comment 12 2018-12-11 15:54:44 PST
Myles C. Maxfield
Comment 13 2018-12-11 22:13:34 PST
Myles C. Maxfield
Comment 14 2018-12-12 17:35:46 PST
Myles C. Maxfield
Comment 15 2018-12-12 19:46:31 PST
Myles C. Maxfield
Comment 16 2018-12-13 04:37:09 PST
Created attachment 357225 [details] First draft
Myles C. Maxfield
Comment 17 2018-12-13 12:34:19 PST
Created attachment 357245 [details] Hooking it up
Myles C. Maxfield
Comment 18 2018-12-13 12:35:38 PST
1. Create the standard library at build time 2. Make sure the standard library parses
Myles C. Maxfield
Comment 19 2018-12-13 15:56:23 PST
Myles C. Maxfield
Comment 20 2018-12-13 17:51:36 PST
Created attachment 357277 [details] Standard Library
Myles C. Maxfield
Comment 21 2018-12-13 23:00:34 PST
Created attachment 357303 [details] Starting to debug
Myles C. Maxfield
Comment 22 2018-12-13 23:54:04 PST
Created attachment 357304 [details] Lexer needs some love
Myles C. Maxfield
Comment 23 2018-12-14 00:05:47 PST
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
Comment 24 2018-12-14 17:34:46 PST
Myles C. Maxfield
Comment 25 2018-12-15 16:50:07 PST
Created attachment 357410 [details] Standard library parses
Myles C. Maxfield
Comment 26 2018-12-17 01:56:47 PST
Created attachment 357430 [details] Finishing up move to Expected
Myles C. Maxfield
Comment 27 2018-12-17 11:15:09 PST
Created attachment 357458 [details] Moved to Expected
Myles C. Maxfield
Comment 28 2018-12-17 19:39:20 PST
Created attachment 357520 [details] Need to improve performance
Myles C. Maxfield
Comment 29 2018-12-18 13:21:08 PST
Myles C. Maxfield
Comment 30 2018-12-19 16:20:29 PST
Created attachment 357749 [details] Needs to build on Sierra
Myles C. Maxfield
Comment 31 2018-12-19 16:44:49 PST
Created attachment 357754 [details] Limit the standard library
Myles C. Maxfield
Comment 32 2018-12-19 17:21:26 PST
Created attachment 357765 [details] Limit the standard library
Myles C. Maxfield
Comment 33 2018-12-20 14:11:06 PST
Created attachment 357869 [details] Compiles on Sierra
Myles C. Maxfield
Comment 34 2018-12-20 16:01:55 PST
Myles C. Maxfield
Comment 35 2018-12-20 16:22:52 PST
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 36 2018-12-20 16:29:35 PST
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
Comment 37 2018-12-20 23:00:19 PST
Saam Barati
Comment 38 2018-12-20 23:21:03 PST
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 39 2018-12-21 00:15:35 PST
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
Comment 40 2018-12-21 00:15:42 PST
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 41 2018-12-21 09:56:36 PST
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
Comment 42 2018-12-21 12:14:28 PST
(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
Comment 43 2018-12-21 13:12:10 PST
Breaking this patch up
Myles C. Maxfield
Comment 44 2018-12-21 13:41:01 PST
Sam Weinig
Comment 45 2018-12-21 19:54:03 PST
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
Comment 46 2019-01-10 19:58:52 PST
Myles C. Maxfield
Comment 47 2019-01-10 23:08:59 PST
(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
Comment 48 2019-01-10 23:26:34 PST
Myles C. Maxfield
Comment 49 2019-01-11 09:35:37 PST
Myles C. Maxfield
Comment 50 2019-01-13 14:17:25 PST
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
Comment 51 2019-01-13 18:11:20 PST
(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
Comment 52 2019-01-15 20:08:43 PST
EWS Watchlist
Comment 53 2019-01-15 20:10:23 PST
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 54 2019-01-15 20:33:21 PST
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 55 2019-01-16 12:26:58 PST
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
Comment 56 2019-01-16 12:27:50 PST
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
Comment 57 2019-01-16 16:14:45 PST
Radar WebKit Bug Importer
Comment 58 2019-01-16 16:16:19 PST
Alex Christensen
Comment 59 2019-01-17 11:26:39 PST
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
Comment 60 2019-01-17 11:46:59 PST
(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 61 2019-01-17 16:06:34 PST
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.
Note You need to log in before you can comment on or make changes to this bug.