[webkit-reviews] review requested: [Bug 193080] [WHLSL] Implement the Type Checker : [Attachment 359019] Patch

[bugzilla-daemon at webkit.org]

Robin Morisset <rmorisset at apple.com> has asked	for review:
Bug 193080: [WHLSL] Implement the Type Checker
https://bugs.webkit.org/show_bug.cgi?id=193080

Attachment 359019: Patch

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




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

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

I've only reviewed the beginning so far (first 400 lines). The rest of the
review will come later.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:120
> +static Optional<AST::NativeFunctionDeclaration>
resolveWithOperatorAnderIndexer(AST::CallExpression& callExpression,
AST::ArrayReferenceType& firstArgument, const Intrinsics& intrinsics)

I don't see in which case the Optional<> result is ever not present?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:122
> +    bool isRestricted = false;

These two booleans could be const I think.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:131
> +static Optional<AST::NativeFunctionDeclaration>
resolveWithOperatorLength(AST::CallExpression& callExpression,
AST::UnnamedType& firstArgument, const Intrinsics& intrinsics)

Same as above.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:133
> +    bool isRestricted = false;

Same as above.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:297
> +	       if (is<AST::PointerType>(unnamedType))

Is it on purpose that PointerTypes are rejected here, but not array reference
types nor array types?
In the spec I have all three forbidden in that position.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:300
> +	   return true;

If the kind is CheckKind::Index, the spec also requires a check that the second
argument is one of uchar, ushort, uint, char, short or int.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:310
> +	       if (is<AST::PointerType>(unnamedType))

Same question as above.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:332
> +    };

I have not carefully checked this bunch of checks to find the matching getter,
they are a bit hard to read. We'll just have to remember to test them.

If the kind is CheckKind::Index, the spec also requires a check that the second
argument is one of uchar, ushort, uint, char, short or int.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:351
> +	       return is<AST::PointerType>(unnamedType) ||
is<AST::ArrayReferenceType>(unnamedType);

This seems weird to me: why would we require that the first parameter of an
ander be a pointer or reference? Don't we want to forbid it instead?