Bug 177303

Created attachment 321439 [details] the patch

Created attachment 321447 [details] the patch

Created attachment 321450 [details] the patch

*** Bug 177098 has been marked as a duplicate of this bug. ***

Comment on attachment 321450 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=321450&action=review r=me with some nits. > Tools/ChangeLog:26 > + Finally, this removes the shader type checker's incomplete reimplementation of isPrimitive. That calls > + isPrimitive now. I don't understand what this is saying. What is "That"? isPrimitive was/is a property. AFAICT, there is no isPrimitive function? > Tools/WebGPUShadingLanguageRI/LateChecker.js:31 > + constructor() > + { > + super(); > + } Nit: you can remove this. Unless you want to be really sure that no arguments are passed to Visitor. > Tools/WebGPUShadingLanguageRI/LateChecker.js:52 > + if (!(node.returnType.type instanceof StructType)) > + throw new WTypeError(node.origin.originString, "Vertex shader " + node.name + " must return a struct."); > + assertPrimitive(node.returnType); Nit: you could probably move this and the one below out of the switch.

(In reply to Keith Miller from comment #5) > Comment on attachment 321450 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321450&action=review > > r=me with some nits. > > > Tools/ChangeLog:26 > > + Finally, this removes the shader type checker's incomplete reimplementation of isPrimitive. That calls > > + isPrimitive now. > > I don't understand what this is saying. What is "That"? isPrimitive was/is a > property. AFAICT, there is no isPrimitive function? There is an isPrimitive getter all over the place. For example: let assertPrimitive = type => { if (!type.isPrimitive) throw new WTypeError(node.origin.originString, "Shader signature cannot include non-primitive type: " + type); } Before, this code used its own probably-mostly-correct reimplementation: class NonNumericSearcher extends Visitor { constructor(name) { super(); this._name = name; } visitArrayRefType(node) { throw new WTypeError(node.origin.originString, shaderFunc.name + " must transitively only have numeric types."); } visitPtrType(node) { throw new WTypeError(node.origin.originString, shaderFunc.name + " must transitively only have numeric types."); } visitTypeRef(node) { super.visitTypeRef(node); node.type.visit(this); } } That was when this code lived in Checker, so the patch doesn't make this super obvious. I'll fix the changelog to refer to this defunct helper class by name. > > > Tools/WebGPUShadingLanguageRI/LateChecker.js:31 > > + constructor() > > + { > > + super(); > > + } > > Nit: you can remove this. Unless you want to be really sure that no > arguments are passed to Visitor. Fixed. > > > Tools/WebGPUShadingLanguageRI/LateChecker.js:52 > > + if (!(node.returnType.type instanceof StructType)) > > + throw new WTypeError(node.origin.originString, "Vertex shader " + node.name + " must return a struct."); > > + assertPrimitive(node.returnType); > > Nit: you could probably move this and the one below out of the switch. Fixed.

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

<rdar://problem/34693183>

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