WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
WIP
(78.14 KB, patch)
2018-12-04 18:40 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(86.42 KB, patch)
2018-12-04 19:31 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(93.11 KB, patch)
2018-12-04 23:24 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(99.49 KB, patch)
2018-12-05 19:09 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(108.98 KB, patch)
2018-12-06 19:42 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(134.77 KB, patch)
2018-12-07 22:07 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(146.82 KB, patch)
2018-12-09 23:36 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(180.15 KB, patch)
2018-12-10 19:34 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(198.39 KB, patch)
2018-12-10 22:39 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(198.44 KB, patch)
2018-12-10 23:30 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(235.12 KB, patch)
2018-12-11 15:54 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(252.46 KB, patch)
2018-12-11 22:13 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(267.56 KB, patch)
2018-12-12 17:35 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(301.90 KB, patch)
2018-12-12 19:46 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
First draft
(334.37 KB, patch)
2018-12-13 04:37 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Hooking it up
(4.76 MB, patch)
2018-12-13 12:34 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(4.93 MB, patch)
2018-12-13 15:56 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Standard Library
(4.05 MB, patch)
2018-12-13 17:51 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Starting to debug
(4.05 MB, patch)
2018-12-13 23:00 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Lexer needs some love
(4.05 MB, patch)
2018-12-13 23:54 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(4.04 MB, patch)
2018-12-14 17:34 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Standard library parses
(4.06 MB, patch)
2018-12-15 16:50 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Finishing up move to Expected
(4.08 MB, patch)
2018-12-17 01:56 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Moved to Expected
(4.08 MB, patch)
2018-12-17 11:15 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Need to improve performance
(4.08 MB, patch)
2018-12-17 19:39 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(4.08 MB, patch)
2018-12-18 13:21 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Needs to build on Sierra
(4.05 MB, patch)
2018-12-19 16:20 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Limit the standard library
(359.28 KB, patch)
2018-12-19 16:44 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Limit the standard library
(359.32 KB, patch)
2018-12-19 17:21 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Compiles on Sierra
(356.44 KB, text/plain)
2018-12-20 14:11 PST
,
Myles C. Maxfield
no flags
Details
Patch
(361.53 KB, patch)
2018-12-20 16:01 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(361.75 KB, patch)
2018-12-20 23:00 PST
,
Myles C. Maxfield
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
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
Details
Patch
(115.77 KB, patch)
2018-12-21 13:41 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(117.97 KB, patch)
2019-01-10 19:58 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(121.38 KB, patch)
2019-01-10 23:26 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(120.88 KB, patch)
2019-01-11 09:35 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(122.00 KB, patch)
2019-01-15 20:08 PST
,
Myles C. Maxfield
dino
: review+
Details
Formatted Diff
Diff
Show Obsolete
(38)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2018-12-03 22:16:11 PST
Created
attachment 356468
[details]
WIP
Myles C. Maxfield
Comment 2
2018-12-04 18:40:25 PST
Created
attachment 356569
[details]
WIP
Myles C. Maxfield
Comment 3
2018-12-04 19:31:30 PST
Created
attachment 356570
[details]
WIP
Myles C. Maxfield
Comment 4
2018-12-04 23:24:30 PST
Created
attachment 356585
[details]
WIP
Myles C. Maxfield
Comment 5
2018-12-05 19:09:42 PST
Created
attachment 356695
[details]
WIP
Myles C. Maxfield
Comment 6
2018-12-06 19:42:12 PST
Created
attachment 356778
[details]
WIP
Myles C. Maxfield
Comment 7
2018-12-07 22:07:19 PST
Created
attachment 356863
[details]
WIP
Myles C. Maxfield
Comment 8
2018-12-09 23:36:20 PST
Created
attachment 356947
[details]
WIP
Myles C. Maxfield
Comment 9
2018-12-10 19:34:40 PST
Created
attachment 357031
[details]
WIP
Myles C. Maxfield
Comment 10
2018-12-10 22:39:27 PST
Created
attachment 357035
[details]
WIP
Myles C. Maxfield
Comment 11
2018-12-10 23:30:25 PST
Created
attachment 357038
[details]
WIP
Myles C. Maxfield
Comment 12
2018-12-11 15:54:44 PST
Created
attachment 357081
[details]
WIP
Myles C. Maxfield
Comment 13
2018-12-11 22:13:34 PST
Created
attachment 357103
[details]
WIP
Myles C. Maxfield
Comment 14
2018-12-12 17:35:46 PST
Created
attachment 357199
[details]
WIP
Myles C. Maxfield
Comment 15
2018-12-12 19:46:31 PST
Created
attachment 357207
[details]
WIP
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
Created
attachment 357268
[details]
WIP
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
Created
attachment 357361
[details]
WIP
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
Created
attachment 357602
[details]
Patch
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
Created
attachment 357889
[details]
Patch
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
Created
attachment 357929
[details]
Patch
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
Created
attachment 357975
[details]
Patch
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
Created
attachment 358866
[details]
WIP
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
Created
attachment 358880
[details]
Patch
Myles C. Maxfield
Comment 49
2019-01-11 09:35:37 PST
Created
attachment 358902
[details]
Patch
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
Created
attachment 359250
[details]
Patch
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
Committed
r240092
: <
https://trac.webkit.org/changeset/240092
>
Radar WebKit Bug Importer
Comment 58
2019-01-16 16:16:19 PST
<
rdar://problem/47333841
>
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.
Top of Page
Format For Printing
XML
Clone This Bug