Bug 177044

Created attachment 321412 [details] the patch

Comment on attachment 321412 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=321412&action=review > Tools/WebGPUShadingLanguageRI/Checker.js:64 > + let shaderType = node; This doesn’t seem right. Should this be shaderType = node.shaderType;? > Tools/WebGPUShadingLanguageRI/Checker.js:85 > switch (node.shaderType) { ... and you can use it here? > Tools/WebGPUShadingLanguageRI/RecursiveTypeChecker.js:44 > + this._visiting.doVisit(node, () => super.visitStructType(node)); So linked lists will work, but struct Foo { Foo x; int y; } won’t. > Tools/WebGPUShadingLanguageRI/Test.js:4436 > + () => checkInt(program, callFunction(program, "foo", [], []), -511), I thought callFunction() caught theTrapError? > Tools/WebGPUShadingLanguageRI/Test.js:4463 > +} Can we test linked lists? > Tools/WebGPUShadingLanguageRI/Visitor.js:-98 > - Node.visit(node.type, this); Why not? Because this type would be visited in some other part of the tree?

(In reply to Myles C. Maxfield from comment #2) > Comment on attachment 321412 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321412&action=review > > > Tools/WebGPUShadingLanguageRI/Checker.js:64 > > + let shaderType = node; > > This doesn’t seem right. Should this be shaderType = node.shaderType;? This should say "let shaderFunc = node" > > > Tools/WebGPUShadingLanguageRI/Checker.js:85 > > switch (node.shaderType) { > > ... and you can use it here? > > > Tools/WebGPUShadingLanguageRI/RecursiveTypeChecker.js:44 > > + this._visiting.doVisit(node, () => super.visitStructType(node)); > > So linked lists will work, but struct Foo { Foo x; int y; } won’t. Yeah > > > Tools/WebGPUShadingLanguageRI/Test.js:4436 > > + () => checkInt(program, callFunction(program, "foo", [], []), -511), > > I thought callFunction() caught theTrapError? I will fix this. We need two versions of callFunction(): - A low-level version that does not catch trap error. - A high-level version that does catch trap error. We need the low-level one to write tests. > > > Tools/WebGPUShadingLanguageRI/Test.js:4463 > > +} > > Can we test linked lists? I can add that. > > > Tools/WebGPUShadingLanguageRI/Visitor.js:-98 > > - Node.visit(node.type, this); > > Why not? Because this type would be visited in some other part of the tree? If we visit it here, then we'll get a JS stack overflow error anytime we try to typecheck a program with a linked list.

(In reply to Filip Pizlo from comment #3) > (In reply to Myles C. Maxfield from comment #2) > > Comment on attachment 321412 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=321412&action=review > > > > > Tools/WebGPUShadingLanguageRI/Checker.js:64 > > > + let shaderType = node; > > > > This doesn’t seem right. Should this be shaderType = node.shaderType;? > > This should say "let shaderFunc = node" > > > > > > Tools/WebGPUShadingLanguageRI/Checker.js:85 > > > switch (node.shaderType) { > > > > ... and you can use it here? > > > > > Tools/WebGPUShadingLanguageRI/RecursiveTypeChecker.js:44 > > > + this._visiting.doVisit(node, () => super.visitStructType(node)); > > > > So linked lists will work, but struct Foo { Foo x; int y; } won’t. > > Yeah > > > > > > Tools/WebGPUShadingLanguageRI/Test.js:4436 > > > + () => checkInt(program, callFunction(program, "foo", [], []), -511), > > > > I thought callFunction() caught theTrapError? > > I will fix this. We need two versions of callFunction(): > > - A low-level version that does not catch trap error. > - A high-level version that does catch trap error. > > We need the low-level one to write tests. Oh snap. This "worked" because you added a TrapException instead of using WTrapError, and then you didn't convert all of the places that used WTrapError to use TrapException. Please use WTrapError. :-) I'll fix that part of your code, and refactor callFunction.

Landed in https://trac.webkit.org/changeset/222328/webkit

<rdar://problem/34694297>

Migrated to https://github.com/gpuweb/WHLSL/issues/130