[webkit-reviews] review granted: [Bug 195682] [WHLSL] Add descriptive error messages : [Attachment 374717] 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 195682: [WHLSL] Add descriptive error messages
https://bugs.webkit.org/show_bug.cgi?id=195682

Attachment 374717: patch

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




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

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:810
> +	   setError(Error("Left hand side of assignment does not match the type
of the right hand side.", readModifyWriteExpression.codeLocation()));

Assignment? Don't you mean read-modify-write?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:887
> +	   setError(Error("Cannot take the address of a value without a type.",
makeArrayReferenceExpression.codeLocation()));

Cannot make an array reference from a value without a type

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:903
> +	       setError(Error("Cannot take the address of a non lvalue.",
makeArrayReferenceExpression.codeLocation()));

Cannot make an array reference from a non-left-value

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:917
> +	   setError(Error("Cannot take the address of a non lvalue.",
makeArrayReferenceExpression.codeLocation()));

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1102
> +	   setError(Error("Cannot return nothing from a non-void function.",
returnStatement.codeLocation()));

I think this is backwards. It has to return void.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1434
> +	   setError(Error("Cannot resolve function call to a concrete callee.
Make sure you are using compatible types.", callExpression.codeLocation()));

I think this should be "Could not find any functions with appropriate name"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1440
> +	   setError(Error("Cannot resolve function call to a concrete callee.
Make sure you are using compatible types.", callExpression.codeLocation()));

Can we add a FIXME and bug to add more detail here? It would be good to know
why all the overrides were rejected.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:154
> +Expected<RenderPrepareResult, String> prepare(String& whlslSource,
RenderPipelineDescriptor& renderPipelineDescriptor)

I think the API should return an Error object, which can have some fields
inside it. The code that actually outputs this stuff (into the inspector) might
want to linkify stuff, etc, and serializing everything to a string loses that
information.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:428
>	   auto callExpression = getterCall(propertyAccessExpression,
relevantAnder, previousLeftValue, indexVariable ? &*indexVariable : nullptr);

getterCall() doesn't actually appear to ever return nullopt :(

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:451
>      auto modificationResult =
modification(WTFMove(lastGetterCallExpression));

Similarly, modification() seems to only return nullopt if getterCall() returns
nullopt, which seems to never happen.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:471
>	   }, indexVariable ? &*indexVariable : nullptr);

setterCall() doesn't actually appear to ever return nullopt :(

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:498
>	   }, indexVariables[indexVariables.size() - 1] ?
&*(indexVariables[indexVariables.size() - 1]) : nullptr);

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:665
> +	   setError(Error("Unexpected lhs value for read modify write
expression.", readModifyWriteExpression.leftValue().codeLocation()));

Base of read modify write expression is a right-value

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:234
> +		   return makeUnexpected(Error("void functions must return
nothing."));

Or not return?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:237
> +		   return makeUnexpected(Error("Non-void functions must return
something."));

in all code paths