Bug 176975 – WSL should support ++, --, +=, and all of those things

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Filip Pizlo]

Patch fhrtchoming.

[Filip Pizlo]

Created attachment 320866 [details]
the patch

[Myles C. Maxfield]

Comment on attachment 320866 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320866&action=review

> Tools/WebGPUShadingLanguageRI/Checker.js:182
> +        node.type = node.argument.visit(this);

Javascript is awesome. LetExpression dresses up as if it were a VariableDecl and JavaScript just makes it work.

> Tools/WebGPUShadingLanguageRI/EBufferBuilder.js:59
> +            node.initializer.visit(this);

lololol. Can we add a test for this?

> Tools/WebGPUShadingLanguageRI/LetExpression.js:42
> +        return this.name + "(" + this.argument + ", " + this.body + ")";

|this| doesn't appear to have a "body" value. I know you set it in the parser, but it would aid readability if you put |body| and |argument| in the constructor (and passed undefined to the constructor if necessary).

> Tools/WebGPUShadingLanguageRI/Parse.js:340
> +    function doIncrement(token, ptr, extraArg)

"do" is a little misleading. Maybe "emit"?

> Tools/WebGPUShadingLanguageRI/Parse.js:342
> +        let args = [new DereferenceExpression(token, VariableRef.wrap(ptr))];

I don't understand why all these Dereference expressions are necessary (lines 342, 352, 362, 413). Why don't we just remove the MakePtrs on lines 359 and 410 and then the Dereferences wouldn't be necessary? Is the reason because, if we did this, it would be impossible to change the value of "b" in the statement "++b"? Because you'd just be changing the LetExpression's ePtr and not the real b?

> Tools/WebGPUShadingLanguageRI/Parse.js:349
> +        

Might want to check that |name| is not exactly equal to "operator"

> Tools/WebGPUShadingLanguageRI/Parse.js:361
> +        let oldValue = new LetExpression(token);

I think I understand this. Cute.

> Tools/WebGPUShadingLanguageRI/Parse.js:407
> +    function finishParsingPreIncrement(token, left, extraArg)

Shouldn't this be called "right"?

> Tools/WebGPUShadingLanguageRI/Parse.js:421
> +        let left = parsePossiblePrefix();

Shouldn't this be called "right"?

[Filip Pizlo]

(In reply to Myles C. Maxfield from comment #2)
> Comment on attachment 320866 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320866&action=review
> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:182
> > +        node.type = node.argument.visit(this);
> 
> Javascript is awesome. LetExpression dresses up as if it were a VariableDecl
> and JavaScript just makes it work.
> 
> > Tools/WebGPUShadingLanguageRI/EBufferBuilder.js:59
> > +            node.initializer.visit(this);
> 
> lololol. Can we add a test for this?

I'll try!

> 
> > Tools/WebGPUShadingLanguageRI/LetExpression.js:42
> > +        return this.name + "(" + this.argument + ", " + this.body + ")";
> 
> |this| doesn't appear to have a "body" value. I know you set it in the
> parser, but it would aid readability if you put |body| and |argument| in the
> constructor (and passed undefined to the constructor if necessary).

Fixed: I initialized them both to null in the constructor.

> 
> > Tools/WebGPUShadingLanguageRI/Parse.js:340
> > +    function doIncrement(token, ptr, extraArg)
> 
> "do" is a little misleading. Maybe "emit"?

Fixed.

> 
> > Tools/WebGPUShadingLanguageRI/Parse.js:342
> > +        let args = [new DereferenceExpression(token, VariableRef.wrap(ptr))];
> 
> I don't understand why all these Dereference expressions are necessary
> (lines 342, 352, 362, 413). Why don't we just remove the MakePtrs on lines
> 359 and 410 and then the Dereferences wouldn't be necessary? Is the reason
> because, if we did this, it would be impossible to change the value of "b"
> in the statement "++b"? Because you'd just be changing the LetExpression's
> ePtr and not the real b?

Yeah, then you wouldn't actually be changing the value of the variable.

> 
> > Tools/WebGPUShadingLanguageRI/Parse.js:349
> > +        
> 
> Might want to check that |name| is not exactly equal to "operator"

Fixed.

> 
> > Tools/WebGPUShadingLanguageRI/Parse.js:361
> > +        let oldValue = new LetExpression(token);
> 
> I think I understand this. Cute.
> 
> > Tools/WebGPUShadingLanguageRI/Parse.js:407
> > +    function finishParsingPreIncrement(token, left, extraArg)
> 
> Shouldn't this be called "right"?

It's left += extraArg.  So, extraArg could be called right, but left is totally left.

> 
> > Tools/WebGPUShadingLanguageRI/Parse.js:421
> > +        let left = parsePossiblePrefix();
> 
> Shouldn't this be called "right"?

Ditto.  Pretty sure it's right.

[Filip Pizlo]

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

[Radar WebKit Bug Importer]

<rdar://problem/34693302>

[Myles C. Maxfield]

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