[webkit-reviews] review granted: [Bug 176975] WSL should support ++, --, +=, and all of those things : [Attachment 320866] the patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 176975: WSL should support ++, --, +=, and all of those things
https://bugs.webkit.org/show_bug.cgi?id=176975

Attachment 320866: the patch

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




--- Comment #2 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 320866
  --> https://bugs.webkit.org/attachment.cgi?id=320866
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"?