[webkit-reviews] review granted: [Bug 195808] [WHLSL] Implement loop expressions : [Attachment 371103] patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 195808: [WHLSL] Implement loop expressions
https://bugs.webkit.org/show_bug.cgi?id=195808

Attachment 371103: patch

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




--- Comment #6 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 371103
  --> https://bugs.webkit.org/attachment.cgi?id=371103
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:231
> +    m_stringBuilder.append(makeString(m_breakOutOfCurrentLoopEarlyVariable,
" = true;\n"));

Can we ASSERT() that the variable is non-empty?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:237
> +    m_stringBuilder.append("break;\n");

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:257
> +    m_stringBuilder.append("while (1) {\n");

Nit: I think while (true) is clearer.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:266
> +    m_stringBuilder.append("} while(0); \n");

Nit: while(false)

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:267
> +    m_stringBuilder.append(makeString("if (",
m_breakOutOfCurrentLoopEarlyVariable, ") break;\n"));

Cool.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:276
> +	   m_stack.takeLast();

Maybe add a short comment about why we need to takeLast()

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:279
> +    m_stringBuilder.append("} \n"); // while (1) { 

Spurious comment?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:284
> +    emitLoop(false, &doWhileLoop.conditional(), nullptr,
doWhileLoop.body());

I think WebKit style usually prefers not to have magic "true" or "false"
values, instead preferring to use an enum class with 2 values.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:297
>	   checkErrorAndVisit(expression);

I know you didn't write this code, but I think we should scope these statements
to just the loop, by surrounding the entire for loop with { }. It doesn't
affect correctness, but it can at least tell us early if we've messed something
up.