| Summary: | Initial reference implementation scaffolding for WebGPU Shading Language prototype | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||||||||||||||||||||||||||||||||
| Component: | WebGPU | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | joepeck, keith_miller, lforschler, mmaxfield, saam, sam, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 176199 | ||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Filip Pizlo
2017-08-23 21:42:40 PDT
Created attachment 318967 [details]
it's a start
Comment on attachment 318967 [details]
it's a start
Cool, I think we should consider using ES6 modules for this though. It will be harder to change to later if we don't start with it now and it will help us enforce some amount of abstraction boundaries.
Created attachment 319008 [details]
a little more
Nothing more fun than a Thursday morning hacking spree.
Created attachment 319056 [details]
more
Fixed component. Created attachment 319065 [details]
core type checker is looking good
Created attachment 319100 [details]
steadily adding expressions
Created attachment 319116 [details]
a bit more things
The AST is rich enough to say things like:
int32 foo(int32 x)
{
return x + 42;
}
But we cannot parse this yet.
Also, I haven't even tried running it.
Created attachment 319122 [details]
started writing the parser
Created attachment 319136 [details]
a bit more
Working through the parser. Almost at the point where it can parse interesting things.
Created attachment 319140 [details]
parser is getting real
Created attachment 319143 [details]
lot more code in the parser
The parser is so dense. It's not done yet.
Created attachment 319147 [details]
more parser
Created attachment 319162 [details]
more
I'm mostly done with the parser.
I'm also starting to notice some weirdnesses in the type system regarding arrays.
Overall it's looking pretty good.
Created attachment 319205 [details]
added more overload resolution machinery
Created attachment 319220 [details]
started working on the interrpeter
Created attachment 319226 [details]
wrote an inliner
That let me do the hash-cons type instantiation, which allows me to allocate LValues.
Created attachment 319229 [details]
more
Created attachment 319244 [details]
inching forward with native functions
Created attachment 319256 [details]
it is written
Now I need to test it.
Created attachment 319259 [details]
starting to debug
Created attachment 319268 [details]
getting close to parsing the first test
Created attachment 319284 [details]
the patch
Comment on attachment 319284 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=319284&action=review Using Source/WebInspectorUI/.eslintrc with ESLint I trimmed down to a list of things that look like legit issues that would cause unexpected behavior or runtime exceptions and a few pedantic things like missing semicolons. Probably would be worth addressing these early otherwise they will just have to be debugged and addressed later when hooking up sections of the code: Tools/ArrayLangRI/ArrayType.js 28:5 error Expected to call 'super()' constructor-super 30:9 error 'this' is not allowed before 'super()' no-this-before-super 31:9 error 'this' is not allowed before 'super()' no-this-before-super 32:9 error 'this' is not allowed before 'super()' no-this-before-super Tools/ArrayLangRI/Block.js 48:51 error Expected an assignment or function call and instead saw an expression no-unused-expressions 48:51 error Unreachable code no-unreachable Tools/ArrayLangRI/ConstexprTypeParameter.js 28:5 error Expected to call 'super()' constructor-super 30:9 error 'this' is not allowed before 'super()' no-this-before-super 31:9 error 'this' is not allowed before 'super()' no-this-before-super 32:9 error 'this' is not allowed before 'super()' no-this-before-super Tools/ArrayLangRI/FuncInstantiator.js 61:13 error 'parameters' is not defined no-undef 62:13 error 'body' is not defined no-undef Tools/ArrayLangRI/FunctionLikeBlock.js 28:5 error Expected to call 'super()' constructor-super 30:9 error 'this' is not allowed before 'super()' no-this-before-super 31:9 error 'this' is not allowed before 'super()' no-this-before-super 32:9 error 'this' is not allowed before 'super()' no-this-before-super 33:9 error 'this' is not allowed before 'super()' no-this-before-super 43:26 error 'parameters' is not defined no-undef 43:48 error 'block' is not defined no-undef 43:64 error 'argumentList' is not defined no-undef Tools/ArrayLangRI/NameContext.js 158:18 error 'map' is assigned a value but never used no-unused-vars - Yeah this does seem peculiar but it might not be wrong. Tools/ArrayLangRI/NameResolver.js 132:37 error 'node' is not defined no-undef 141:39 error 'node' is not defined no-undef 157:35 error 'origin' is not defined no-undef 163:39 error 'origin' is not defined no-undef 165:39 error 'origin' is not defined no-undef Tools/ArrayLangRI/NativeTypeInstance.js 28:5 error Expected to call 'super()' constructor-super 30:9 error 'this' is not allowed before 'super()' no-this-before-super 31:9 error 'this' is not allowed before 'super()' no-this-before-super Tools/ArrayLangRI/Parse.js 223:43 error Missing semicolon semi 239:13 error Expected a conditional expression and instead saw an assignment no-cond-assign 475:60 error 'term' is not defined no-undef 544:48 error Missing semicolon semi Tools/ArrayLangRI/Protocol.js 28:5 error Expected to call 'super()' constructor-super 30:9 error 'this' is not allowed before 'super()' no-this-before-super 31:9 error 'this' is not allowed before 'super()' no-this-before-super Tools/ArrayLangRI/ProtocolDecl.js 30:28 error Missing semicolon semi 78:21 error 'signature' is not defined no-undef 88:18 error 'abstractSignature' is assigned a value but never used no-unused-vars Tools/ArrayLangRI/PtrType.js 30:13 error Unexpected negating the left operand of 'instanceof' operator no-unsafe-negation Tools/ArrayLangRI/Rewriter.js 51:53 error 'typeParameter' is not defined no-undef - This one looks like a typo Tools/ArrayLangRI/StructType.js 28:5 error Expected to call 'super()' constructor-super 30:9 error 'this' is not allowed before 'super()' no-this-before-super 31:9 error 'this' is not allowed before 'super()' no-this-before-super 32:9 error 'this' is not allowed before 'super()' no-this-before-super 33:9 error 'this' is not allowed before 'super()' no-this-before-super Tools/ArrayLangRI/TypeOrVariableRef.js 28:5 error Expected to call 'super()' constructor-super 30:9 error 'this' is not allowed before 'super()' no-this-before-super 31:9 error 'this' is not allowed before 'super()' no-this-before-super Tools/ArrayLangRI/TypeRef.js 47:32 error 'type' is not defined no-undef 47:39 error 'type' is not defined no-undef Tools/ArrayLangRI/TypeVariable.js 28:5 error Expected to call 'super()' constructor-super 30:9 error 'this' is not allowed before 'super()' no-this-before-super 31:9 error 'this' is not allowed before 'super()' no-this-before-super 32:9 error 'this' is not allowed before 'super()' no-this-before-super Tools/ArrayLangRI/Visitor.js 89:18 error 'typeParamter' is assigned a value but never used no-unused-vars 90:13 error 'typeParameter' is not defined no-undef > Tools/ArrayLangRI/ALSyntaxError.js:23 > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. The trailing whitespace in these copyright comments is like a small virus that spreads throughout our codebase =) > Tools/ArrayLangRI/AddressSpace.js:27 > +const addressSpaces = ["constant", "device", "threadgroup", "thread"]; In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually. > Tools/ArrayLangRI/Block.js:48 > + return "{ " + this.statements.join("; "); + "; }"; The semicolon after the join breaks the intended behavior here. > Tools/ArrayLangRI/CommaExpression.js:44 > +}; ESLint had some warnings about these as unnecessary semicolons. You have them sometimes and sometimes you don't. Up to you if you want to clean them up. > Tools/ArrayLangRI/FuncSignature.js:26 > +"use strict"; > + This file is empty! Drop it? > Tools/ArrayLangRI/NameResolver.js:34 > + // It's totally OK to instantiate this with zero arguments (i.e. passing undefined for both of > + // them) if you're creating a Checker to visit a Program. This alludes to multiple arguments but that is no longer the case. Seems the comment is stale. > Tools/ArrayLangRI/ProtocolDecl.js:40 > + // FIXME: Check this! (OOPS!) Heh, should this be tweaked now or later? Comment on attachment 319284 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=319284&action=review Partial review, not done. > Tools/ChangeLog:3 > + Initial reference implementation scaffolding for ArrayLang WebGPU Shading Language > Tools/ChangeLog:9 > + experimental shader language we are calling ArrayLang for now. Ditto. > Tools/ChangeLog:17 > + protocol Addable { Addable operator+(Addable, Addable); } And "int operator+(int, int);" > Tools/ChangeLog:24 > + Each variable behaves as if it was declared "static", with one copy per type instantiation. Using C++'s notion of "static" variables >> Tools/ArrayLangRI/AddressSpace.js:27 >> +const addressSpaces = ["constant", "device", "threadgroup", "thread"]; > > In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually. We don't want to run this in a browser eventually. This is a proof-of-concept. > Tools/ArrayLangRI/ArrayRefType.js:36 > + if (this.addressSpace != "thread") > + return false; This looks like a great jumping-off point for me. (In reply to Myles C. Maxfield from comment #25) > Comment on attachment 319284 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319284&action=review > > Partial review, not done. > > > Tools/ChangeLog:3 > > + Initial reference implementation scaffolding for ArrayLang > > WebGPU Shading Language > > > Tools/ChangeLog:9 > > + experimental shader language we are calling ArrayLang for now. > > Ditto. Let’s come up with a shorter name, or use ArrayLang in the initial commit. Renaming is easy. > > > Tools/ChangeLog:17 > > + protocol Addable { Addable operator+(Addable, Addable); } > > And "int operator+(int, int);" Not sure what you mean. That operator is provided by the system. I’m saying that the user could write an Addable protocol, as an example of what the typesyatem is capable of. > > > Tools/ChangeLog:24 > > + Each variable behaves as if it was declared "static", with one copy per type instantiation. > > Using C++'s notion of "static" variables > > >> Tools/ArrayLangRI/AddressSpace.js:27 > >> +const addressSpaces = ["constant", "device", "threadgroup", "thread"]; > > > > In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually. > > We don't want to run this in a browser eventually. This is a > proof-of-concept. This reference implementation will run in a browser in the sense that we will make it a page you can navigate to. > > > Tools/ArrayLangRI/ArrayRefType.js:36 > > + if (this.addressSpace != "thread") > > + return false; > > This looks like a great jumping-off point for me. > > > In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually. > > > > We don't want to run this in a browser eventually. This is a proof-of-concept. > > This reference implementation will run in a browser in the sense that we > will make it a page you can navigate to. I just tested this and a global const variable defined in one <script> was accessible in another <script>. So it appears my comment is incorrect (or perhaps just outdated). I do know we had to make a change to Web Inspector code for exactly this situation in the past: https://trac.webkit.org/changeset/187012/webkit#file82 However, many things could have changed with global `const` behavior between then (the original introduction) and now! (In reply to Joseph Pecoraro from comment #27) > > > > In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually. > > > > > > We don't want to run this in a browser eventually. This is a proof-of-concept. > > > > This reference implementation will run in a browser in the sense that we > > will make it a page you can navigate to. > > I just tested this and a global const variable defined in one <script> was > accessible in another <script>. So it appears my comment is incorrect (or > perhaps just outdated). Yeah this is the specced behavior. > > I do know we had to make a change to Web Inspector code for exactly this > situation in the past: > https://trac.webkit.org/changeset/187012/webkit#file82 > > However, many things could have changed with global `const` behavior between > then (the original introduction) and now! Yeah Yue behavior did change. I originally implemented it incorrectly, which had let/const visible only per script. I later fixed it by making top level let/const visible across scripts. Comment on attachment 319284 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=319284&action=review At a high level, this seems fine. I really appreciate the current design's primary goal being clarity. I'd like to take this and run with it; there are lots of ideas I have for what we can do next with it. It doesn't seem that being extremely detail-oriented would be valuable for this first patch, as this is intended to be platform for further evolution of the language. There is a chance that some piece of this code will end up in production, but that chance is slim. > Tools/ArrayLangRI/EBufferBuilder.js:33 > + // NOTE: I don't think that this is necessary. FIXME? Comment on attachment 319284 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=319284&action=review It's Keith's beddy-bye time, I'm tentatively r-ing based on my comments. I'll read more details tomorrow to make sure I understand correctly. >>> Tools/ChangeLog:3 >>> + Initial reference implementation scaffolding for ArrayLang >> >> WebGPU Shading Language > > Let’s come up with a shorter name, or use ArrayLang in the initial commit. Renaming is easy. Why not WebSL? Are there any shader languages that wouldn't be for the GPU? >>> Tools/ChangeLog:24 >>> + Each variable behaves as if it was declared "static", with one copy per type instantiation. >> >> Using C++'s notion of "static" variables > > This reference implementation will run in a browser in the sense that we will make it a page you can navigate to. I don't like this semantics, unless it's a compile error to use an uninitialized value. In fact, I'll r- on this point alone. e.g. int foo() { int a; return a++; } would return a different value on each invocation. >> Tools/ArrayLangRI/ALSyntaxError.js:23 >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > The trailing whitespace in these copyright comments is like a small virus that spreads throughout our codebase =) Indeed, pizlo needs to install https://github.com/lewang/ws-butler. I recommended it to him previously but he chose to ignore me :( >>> Tools/ArrayLangRI/AddressSpace.js:27 >>> +const addressSpaces = ["constant", "device", "threadgroup", "thread"]; >> >> In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually. > > We don't want to run this in a browser eventually. This is a proof-of-concept. Unless I have misunderstood you Myles, I think we do want this to run in a browser. My understanding is this would be more or less a reference implementation. (In reply to Keith Miller from comment #30) > Comment on attachment 319284 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319284&action=review > > It's Keith's beddy-bye time, I'm tentatively r-ing based on my comments. > I'll read more details tomorrow to make sure I understand correctly. Can you please undo that? The purpose of landing this is so that someone other than me can make changes. We are blocked on this landing. Using r- to indicate that you don’t understand the semantics is not really great since this is a super rough draft. I would have advocated landing as is even if I did not yet have a story for how variables get initialized. It’s great if stuff like that happens after this lands so that I’m on the only one working on it. > > >>> Tools/ChangeLog:3 > >>> + Initial reference implementation scaffolding for ArrayLang > >> > >> WebGPU Shading Language > > > > Let’s come up with a shorter name, or use ArrayLang in the initial commit. Renaming is easy. > > Why not WebSL? Are there any shader languages that wouldn't be for the GPU? > > >>> Tools/ChangeLog:24 > >>> + Each variable behaves as if it was declared "static", with one copy per type instantiation. > >> > >> Using C++'s notion of "static" variables > > > > This reference implementation will run in a browser in the sense that we will make it a page you can navigate to. > > I don't like this semantics, unless it's a compile error to use an > uninitialized value. There are no uniniyialized values. Variables get initialized to the default value for the type at the moment the variable declaration executes. > In fact, I'll r- on this point alone. e.g. > > int foo() { > int a; > return a++; > } > > would return a different value on each invocation. This always returns zero. > > >> Tools/ArrayLangRI/ALSyntaxError.js:23 > >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > > > The trailing whitespace in these copyright comments is like a small virus that spreads throughout our codebase =) > > Indeed, pizlo needs to install https://github.com/lewang/ws-butler. I > recommended it to him previously but he chose to ignore me :( > > >>> Tools/ArrayLangRI/AddressSpace.js:27 > >>> +const addressSpaces = ["constant", "device", "threadgroup", "thread"]; > >> > >> In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually. > > > > We don't want to run this in a browser eventually. This is a proof-of-concept. > > Unless I have misunderstood you Myles, I think we do want this to run in a > browser. My understanding is this would be more or less a reference > implementation. (In reply to Filip Pizlo from comment #31) > (In reply to Keith Miller from comment #30) > > Comment on attachment 319284 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=319284&action=review > > > > It's Keith's beddy-bye time, I'm tentatively r-ing based on my comments. > > I'll read more details tomorrow to make sure I understand correctly. > > Can you please undo that? The purpose of landing this is so that someone > other than me can make changes. We are blocked on this landing. Using r- to > indicate that you don’t understand the semantics is not really great since > this is a super rough draft. I would have advocated landing as is even if I > did not yet have a story for how variables get initialized. It’s great if > stuff like that happens after this lands so that I’m on the only one working > on it. > That's a fair point. My goal was to make my objection adamantly clear. If the patch landed in it's "old" state (I use quotes because it appears I was mistaken), I was concerned that the language might proceed with the "old" semantics. Such a language decision would have been a disaster in my opinion. > > > > >>> Tools/ChangeLog:3 > > >>> + Initial reference implementation scaffolding for ArrayLang > > >> > > >> WebGPU Shading Language > > > > > > Let’s come up with a shorter name, or use ArrayLang in the initial commit. Renaming is easy. > > > > Why not WebSL? Are there any shader languages that wouldn't be for the GPU? > > > > >>> Tools/ChangeLog:24 > > >>> + Each variable behaves as if it was declared "static", with one copy per type instantiation. > > >> > > >> Using C++'s notion of "static" variables > > > > > > This reference implementation will run in a browser in the sense that we will make it a page you can navigate to. > > > > I don't like this semantics, unless it's a compile error to use an > > uninitialized value. > > There are no uniniyialized values. Variables get initialized to the default > value for the type at the moment the variable declaration executes. I see, I misunderstood your statement as a statement about how the language would be specified rather than an implementation detail of the interpreter. > > > In fact, I'll r- on this point alone. e.g. > > > > int foo() { > > int a; > > return a++; > > } > > > > would return a different value on each invocation. > > This always returns zero. > > > > > >> Tools/ArrayLangRI/ALSyntaxError.js:23 > > >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > > > > > The trailing whitespace in these copyright comments is like a small virus that spreads throughout our codebase =) > > > > Indeed, pizlo needs to install https://github.com/lewang/ws-butler. I > > recommended it to him previously but he chose to ignore me :( > > > > >>> Tools/ArrayLangRI/AddressSpace.js:27 > > >>> +const addressSpaces = ["constant", "device", "threadgroup", "thread"]; > > >> > > >> In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually. > > > > > > We don't want to run this in a browser eventually. This is a proof-of-concept. > > > > Unless I have misunderstood you Myles, I think we do want this to run in a > > browser. My understanding is this would be more or less a reference > > implementation. (In reply to Joseph Pecoraro from comment #24) > Comment on attachment 319284 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319284&action=review > > Using Source/WebInspectorUI/.eslintrc with ESLint I trimmed down to a list > of things that look like legit issues that would cause unexpected behavior > or runtime exceptions and a few pedantic things like missing semicolons. > Probably would be worth addressing these early otherwise they will just have > to be debugged and addressed later when hooking up sections of the code: > > Tools/ArrayLangRI/ArrayType.js > 28:5 error Expected to call 'super()' constructor-super > 30:9 error 'this' is not allowed before 'super()' > no-this-before-super > 31:9 error 'this' is not allowed before 'super()' > no-this-before-super > 32:9 error 'this' is not allowed before 'super()' > no-this-before-super > > Tools/ArrayLangRI/Block.js > 48:51 error Expected an assignment or function call and instead saw > an expression no-unused-expressions > 48:51 error Unreachable code > no-unreachable > > Tools/ArrayLangRI/ConstexprTypeParameter.js > 28:5 error Expected to call 'super()' > constructor-super > 30:9 error 'this' is not allowed before 'super()' > no-this-before-super > 31:9 error 'this' is not allowed before 'super()' > no-this-before-super > 32:9 error 'this' is not allowed before 'super()' > no-this-before-super > > Tools/ArrayLangRI/FuncInstantiator.js > 61:13 error 'parameters' is not defined no-undef > 62:13 error 'body' is not defined no-undef > > Tools/ArrayLangRI/FunctionLikeBlock.js > 28:5 error Expected to call 'super()' > constructor-super > 30:9 error 'this' is not allowed before 'super()' > no-this-before-super > 31:9 error 'this' is not allowed before 'super()' > no-this-before-super > 32:9 error 'this' is not allowed before 'super()' > no-this-before-super > 33:9 error 'this' is not allowed before 'super()' > no-this-before-super > 43:26 error 'parameters' is not defined no-undef > 43:48 error 'block' is not defined no-undef > 43:64 error 'argumentList' is not defined no-undef > > Tools/ArrayLangRI/NameContext.js > 158:18 error 'map' is assigned a value but never used no-unused-vars > - Yeah this does seem peculiar but it might not be wrong. > > Tools/ArrayLangRI/NameResolver.js > 132:37 error 'node' is not defined no-undef > 141:39 error 'node' is not defined no-undef > 157:35 error 'origin' is not defined no-undef > 163:39 error 'origin' is not defined no-undef > 165:39 error 'origin' is not defined no-undef > > Tools/ArrayLangRI/NativeTypeInstance.js > 28:5 error Expected to call 'super()' > constructor-super > 30:9 error 'this' is not allowed before 'super()' > no-this-before-super > 31:9 error 'this' is not allowed before 'super()' > no-this-before-super > > Tools/ArrayLangRI/Parse.js > 223:43 error Missing semicolon > semi > 239:13 error Expected a conditional expression and instead saw an > assignment no-cond-assign > 475:60 error 'term' is not defined > no-undef > 544:48 error Missing semicolon > semi > > Tools/ArrayLangRI/Protocol.js > 28:5 error Expected to call 'super()' constructor-super > 30:9 error 'this' is not allowed before 'super()' > no-this-before-super > 31:9 error 'this' is not allowed before 'super()' > no-this-before-super > > Tools/ArrayLangRI/ProtocolDecl.js > 30:28 error Missing semicolon > semi > 78:21 error 'signature' is not defined > no-undef > 88:18 error 'abstractSignature' is assigned a value but never used > no-unused-vars > > Tools/ArrayLangRI/PtrType.js > 30:13 error Unexpected negating the left operand of 'instanceof' > operator no-unsafe-negation > > Tools/ArrayLangRI/Rewriter.js > 51:53 error 'typeParameter' is not defined no-undef > - This one looks like a typo > > Tools/ArrayLangRI/StructType.js > 28:5 error Expected to call 'super()' constructor-super > 30:9 error 'this' is not allowed before 'super()' > no-this-before-super > 31:9 error 'this' is not allowed before 'super()' > no-this-before-super > 32:9 error 'this' is not allowed before 'super()' > no-this-before-super > 33:9 error 'this' is not allowed before 'super()' > no-this-before-super > > Tools/ArrayLangRI/TypeOrVariableRef.js > 28:5 error Expected to call 'super()' > constructor-super > 30:9 error 'this' is not allowed before 'super()' > no-this-before-super > 31:9 error 'this' is not allowed before 'super()' > no-this-before-super > > Tools/ArrayLangRI/TypeRef.js > 47:32 error 'type' is not defined no-undef > 47:39 error 'type' is not defined no-undef > > Tools/ArrayLangRI/TypeVariable.js > 28:5 error Expected to call 'super()' > constructor-super > 30:9 error 'this' is not allowed before 'super()' > no-this-before-super > 31:9 error 'this' is not allowed before 'super()' > no-this-before-super > 32:9 error 'this' is not allowed before 'super()' > no-this-before-super > > Tools/ArrayLangRI/Visitor.js > 89:18 error 'typeParamter' is assigned a value but never used > no-unused-vars > 90:13 error 'typeParameter' is not defined > no-undef I fixed almost all of these. I think that this is OK, so I left it: if (token = parseThings()) doThings(token); > > > Tools/ArrayLangRI/ALSyntaxError.js:23 > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > The trailing whitespace in these copyright comments is like a small virus > that spreads throughout our codebase =) > > > Tools/ArrayLangRI/AddressSpace.js:27 > > +const addressSpaces = ["constant", "device", "threadgroup", "thread"]; > > In a browser, I think the the lexical nature of the `const` would prevent > this constant from being used outside of this file. Same with the cases of > just `class Foo { ... };`. In your case I think it all works in because > All.js just inline `load(...)`s all the individual files. You don't need to > change, just something to consider if you want to try running this in a > browser eventually. > > > Tools/ArrayLangRI/Block.js:48 > > + return "{ " + this.statements.join("; "); + "; }"; > > The semicolon after the join breaks the intended behavior here. Yeah! Ha! I fixed it. > > > Tools/ArrayLangRI/CommaExpression.js:44 > > +}; > > ESLint had some warnings about these as unnecessary semicolons. You have > them sometimes and sometimes you don't. Up to you if you want to clean them > up. Ooops. > > > Tools/ArrayLangRI/FuncSignature.js:26 > > +"use strict"; > > + > > This file is empty! Drop it? Yeah. I dropped it. > > > Tools/ArrayLangRI/NameResolver.js:34 > > + // It's totally OK to instantiate this with zero arguments (i.e. passing undefined for both of > > + // them) if you're creating a Checker to visit a Program. > > This alludes to multiple arguments but that is no longer the case. Seems the > comment is stale. It's a valid comment, I just needed to change "Checker" to "NameResolver". > > > Tools/ArrayLangRI/ProtocolDecl.js:40 > > + // FIXME: Check this! (OOPS!) > > Heh, should this be tweaked now or later? I already implemented that in Checker! I removed the FIXME. (In reply to Myles C. Maxfield from comment #29) > Comment on attachment 319284 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319284&action=review > > At a high level, this seems fine. I really appreciate the current design's > primary goal being clarity. I'd like to take this and run with it; there are > lots of ideas I have for what we can do next with it. It doesn't seem that > being extremely detail-oriented would be valuable for this first patch, as > this is intended to be platform for further evolution of the language. There > is a chance that some piece of this code will end up in production, but that > chance is slim. > > > Tools/ArrayLangRI/EBufferBuilder.js:33 > > + // NOTE: I don't think that this is necessary. > > FIXME? Yup, I removed it. It wasn't necessary. (In reply to Keith Miller from comment #2) > Comment on attachment 318967 [details] > it's a start > > Cool, I think we should consider using ES6 modules for this though. It will > be harder to change to later if we don't start with it now and it will help > us enforce some amount of abstraction boundaries. Now that it's checked in, is that something that would be easy for you to do? > I think that this is OK, so I left it:
>
> if (token = parseThings())
> doThings(token);
Oops, yeah I meant to cull those particular warnings and must have missed one.
Thanks for addressing the rest!
Migrated to https://github.com/gpuweb/WHLSL/issues/12 |