[webkit-reviews] review granted: [Bug 236655] [WGSL] Add enough of the AST for the simplest shaders : [Attachment 452069] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 236655: [WGSL] Add enough of the AST for the simplest shaders
https://bugs.webkit.org/show_bug.cgi?id=236655

Attachment 452069: Patch

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




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

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

r=me, but please consider the LFC approach. Also please don't land until EWS
says it can build.

> Source/WebGPU/WGSL/AST/Attribute.h:35
> +class Attribute : public ASTNode

Attributes are ASTNodes? Why?

> Source/WebGPU/WGSL/AST/Expression.h:60
> +    bool isBoolLiteral() const { return kind() == Kind::BoolLiteral; }
> +    bool isInt32Literal() const { return kind() == Kind::Int32Literal; }
> +    bool isUInt32Literal() const { return kind() == Kind::Uint32Literal; }
> +    bool isFloat32Literal() const { return kind() == Kind::Float32Literal; }
> +    bool isIdentifier() const { return kind() == Kind::Identifier; }
> +    bool isStructureAccess() const { return kind() == Kind::StructureAccess;
}
> +    bool isTypeConversion() const { return kind() == Kind::TypeConversion; }

We usually use virtual functions for these instead of an explicit m_kind.
What's the reason you prefer to spend the memory?

> Source/WebGPU/WGSL/AST/FunctionDecl.h:51
> +		       return { CompilationMessage("It is not valid to apply
the builtin attribute twice to a parameter", attr->span()) };

Do you really want to do validation checking inside the AST nodes?

LFC uses a design that clearly separates the data model from the controller
that operates on that data model. It uses things like explicit Builder classes
rather than calling methods on the AST nodes themselves. I think that's a
better design, because 1) It makes it clear from types involved which operation
is being performed in which stage in the compiler, so it's always clear where
invariants hold and when it's legal to call certain functions 2) it makes code
easier to read (and, I would claim, write) by having each file narrowly contain
a focused set of methods, rather than a sprawl of whatever anything might want
to do on an ASTNode 3) By making stronger separation of functionality, we can
more easily reason about the code at the macro level, for example, to some day
try to run some parts of the compiler in parallel (which may not be that useful
for a WGSL compiler, but has seen some amazing benefits in LFC).

> Source/WebGPU/WGSL/AST/Shader.h:37
> +class Shader : ASTNode {

I would call this ShaderModule. Conceptually I think of "shaders" as individual
entry points.

> Source/WebGPU/WGSL/AST/VariableQualifier.h:46
> +class VariableQualifier : public ASTNode {

Ditto about not understanding why this is an ASTNode and not just a member
field of a variable