[webkit-reviews] review granted: [Bug 201251] [WHLSL] Add back a version of the property resolver : [Attachment 378251] patch

[bugzilla-daemon at webkit.org]

Robin Morisset <rmorisset at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 201251: [WHLSL] Add back a version of the property resolver
https://bugs.webkit.org/show_bug.cgi?id=201251

Attachment 378251: patch

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




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

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

r=me

> Source/WebCore/ChangeLog:9
> +	   The goal of the new property resolver phase is two allow two things:

two allow => to allow

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:90
> +	   while (true) {

This pattern of
```
while(true) {
..
chain.append(current);
if (is<AST::PropertyAccessExpression>(current.base()))
  continue;
Tons of code that only executes once
break;
}
```
is not super readable. It is probably not worth modifying now, but I am
curious, why not just use the loop to accumulate the chain:
```
while (is<AST::PropertyAccessExpression>(current.base())) {
  append to chain
  current = *currentPtr;
}
tons of code that manipulate current but are now outside of the loop
```

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:245
> +	   RELEASE_ASSERT(m_variables.isEmpty());

There is no guarantee that the WTFMove later on will empty-out m_variables.
I think you need to either empty it by hand, or more simply to construct a
different PropertyResolver for each function definition in resolveProperties.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:63
> +    ReadModifyWriteExpression(CodeLocation location, UniqueRef<Expression>
leftValue, UniqueRef<VariableDeclaration> oldValue,
UniqueRef<VariableDeclaration> newValue)

I am unclear on whether it is better to pass UniqueRef<Expression> or
UniqueRef<Expression>&& here.