[webkit-reviews] review canceled: [Bug 199726] [WHLSL] Desugar for loops and while loops : [Attachment 373983] Patch

[bugzilla-daemon at webkit.org]

Robin Morisset <rmorisset at apple.com> has canceled Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 199726: [WHLSL] Desugar for loops and while loops
https://bugs.webkit.org/show_bug.cgi?id=199726

Attachment 373983: Patch

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




--- Comment #3 from Robin Morisset <rmorisset at apple.com> ---
Comment on attachment 373983
  --> https://bugs.webkit.org/attachment.cgi?id=373983
Patch

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

>> Source/WebCore/ChangeLog:8
>> +	    This patch makes loops behave a lot more similarly to the spec.
> 
> Is there a behavior change or a performance change? Is it easier to hack on
them if they are closer? Is it easier to have confidence that our
implementation matches the spec? What is the purpose of this change?

It mostly increases confidence that our implementation matches the spec. It
also makes the implementation simpler, and should have a tiny performance win.

>> Source/WebCore/ChangeLog:11
>> +	    by putting any initializer in a block around the loop, putting true
in the condition if there is none, and putting any litteral in the increment if
there is none.
> 
> And nothing happens with do-while loops, I guess?

Right, do-while loops are not desugarable to for loops.

>> Source/WebCore/ChangeLog:14
>> +	    So I also added a small fix for this.
> 
> This is a little scary. I wouldn't expect this to work. How do you type check
an empty comma expression? We type check from the leaves up to the root, but an
empty comma expression would be a leaf with no type. It seems to me like having
an empty comma expression should remain an error.

I simply type check them to type void. I've since realized that I can keep
empty comma expressions an error, and instead make ";" generate an empty block.
It is both closer to spec, and more obviously correct. I will do this change.

>> Source/WebCore/ChangeLog:18
>> +	    So I removed this unnecessary work, and fixed the parser to
correctly forbid such naked variable declarations.
> 
> Is this tested?

Yes, I verified it does not break any test, but you are right that I should
also add a test that naked variable declarations are properly rejected. Will
do.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1200
>> +	auto increment = makeUniqueRef<AST::BooleanLiteral>(*origin, true); //
FIXME: NullLiteral would make more sense, but is buggy right now. Anything
side-effect free is fine.
> 
> Is it buggy?
> 
> Instead of dumping unnecessary expressions in the for loop, why don't we just
make the increment nullable? Dumping a "true" in there seems worse.

NullLiteral is currently buggy yes. It should be fixed, but this patch is
already big enough.

Keeping the increment nullable would work, but it means an extra null check
everywhere we touch the loop, and nasty bugs if we forget it anywhere. Making
both the increment and the condition non-nullable is just a simplification.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1260
>> +auto Parser::parseStatement(bool allowVariableDeclarations) ->
Expected<UniqueRef<AST::Statement>, Error>
> 
> I don't see this ever called with allowVariableDeclarations set to "true".
Does it need to exist?

It is used in parseBlockBody (see line 1066).