WSL should have some post-instantiation type checking

[litherum]

Migrated from https://bugs.webkit.org/show_bug.cgi?id=177303:

At 2017-09-21T15:50:45Z, [email protected] wrote:
There are certain things that are easier to check after type instantiation.

Shader type checking is definitely in that category.

[litherum]

At 2017-09-21T16:37:47Z, [email protected] wrote:
Created attachment 321439
the patch

[litherum]

At 2017-09-21T17:45:06Z, [email protected] wrote:
Created attachment 321447
the patch

[litherum]

At 2017-09-21T17:53:54Z, [email protected] wrote:
Created attachment 321450
the patch

[litherum]

At 2017-09-21T21:15:27Z, [email protected] wrote:
*** Bug 177098 has been marked as a duplicate of this bug. ***

[litherum]

At 2017-09-21T21:28:55Z, [email protected] wrote:
Comment on attachment 321450
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.

[litherum]

At 2017-09-21T21:33:03Z, [email protected] wrote:
(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.

[litherum]

At 2017-09-21T21:36:08Z, [email protected] wrote:
Landed in https://trac.webkit.org/changeset/222351/webkit

[litherum]

At 2017-09-27T19:23:25Z, [email protected] wrote:
rdar://problem/34693183