[webkit-reviews] review granted: [Bug 187735] Adding WHLSL compiler : [Attachment 345174] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Thomas Denney <tdenney at apple.com>'s
request for review:
Bug 187735: Adding WHLSL compiler
https://bugs.webkit.org/show_bug.cgi?id=187735

Attachment 345174: Patch

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




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 345174
  --> https://bugs.webkit.org/attachment.cgi?id=345174
Patch

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

Can we have some tests that check input v expected output?

> Tools/WebGPUShadingLanguageRI/Metal/ArrayRefDefinition.ts:26
> +/// Writes the definitions of the structures used for the different array
ref types present in the program

While this isn't C++ code, and also isn't JS code, we should probably still
follow the coding guidelines. That would mean using // and ending the sentence
with a full stop (period). The same goes for other comments in the patch.

> Tools/WebGPUShadingLanguageRI/Metal/Deinliner.ts:48
> +	   if (a == ReturnStyle.Always || a == ReturnStyle.SometimesEarly) {
> +	       return a;
> +	   } else {
> +	       return b;
> +	   }

Single line conditionals.

> Tools/WebGPUShadingLanguageRI/Metal/Deinliner.ts:138
> +	       if (result == ReturnStyle.Always) {
> +		   return ReturnStyle.SometimesEarly;
> +	       }

Single line conditionals don't use {} in WebKit.

> Tools/WebGPUShadingLanguageRI/Metal/Deinliner.ts:186
> +    const returnStyle = returnStyleForNodeOrNull(node);
> +    return returnStyle != ReturnStyle.SometimesEarly;

No need for temporary local variable.

> Tools/WebGPUShadingLanguageRI/Metal/FindTopLevelTypeAttributes.ts:69
> +	   case "vertex":
> +	       this.visitVertexShader(func);
> +	   case "fragment":
> +	       this.visitFragmentShader(func);

I assume TypeScript doesn't need break in cases?

> Tools/WebGPUShadingLanguageRI/Metal/FindTopLevelTypeAttributes.ts:78
> +	   for (let param of func.parameters) {
> +	       this.attributesForType(param.type).isVertexAttribute = true;
> +	   }

Single line statement.

> Tools/WebGPUShadingLanguageRI/Metal/FindTopLevelTypeAttributes.ts:85
> +	   for (let param of func.parameters) {
> +	       this.attributesForType(param.type).isVertexOutputOrFragmentInput
= true;
> +	   }

Ditto.

> Tools/WebGPUShadingLanguageRI/Metal/LineNumbers.ts:29
> +    const lineNos = [];
> +    let lineNo = 1;

Use lineNumbers and lineNumber.

> Tools/WebGPUShadingLanguageRI/Metal/LineNumbers.ts:34
> +	   if (src[i] == '\n') {
> +	       lineNo++;
> +	   }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.ts:60
> +	   for (let param of this._func.parameters) {
> +	       map.set(param.name, `P${counter++}`);
> +	   }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.ts:69
> +	   for (let [paramName, param] of this.paramMap) {
> +	       params.push(paramName);
> +	   }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.ts:85
> +	   if (this._func.shaderType === "vertex" || this._func.shaderType ===
"fragment") {
> +	       declLine += `${this._func.shaderType} `;
> +	   }

Should you return if it isn't one of these types?

> Tools/WebGPUShadingLanguageRI/Metal/MSLLexer.ts:87
> +		       for (let innerTok of this._lex(commentText,
this.commentMatchers)) {
> +			   tokens.push(innerTok);
> +		       }

SIngle line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLLexer.ts:105
> +		       } else {
> +			   tokens.push(new Token(matcher.type, match[0]));
> +		       }

Ditto.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:74
> +	   if (this.ancestorReturnType.name === "void") {
> +	       this._add(`return;`);
> +	   } else {
> +	       const resultVar = this._fresh();
> +	      
this._add(`${this._typeNamer.mslTypeName(this.ancestorReturnType)}
${resultVar};`);
> +	       this._zeroInitialize(this.ancestorReturnType, resultVar);
> +	       this._add(`return ${resultVar};`);
> +	   }

For things like this we normally do early returns. e.g.

if (this.ancestorReturnType.name === "void") {
  this._add(`return;`);
  return;
}

const resultVar....

Also, you don't need a `` string for the "return". I'm not sure there is really
a performance win though.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:91
> +		   if (node.name == "bool") {
> +		       emitter._add(`${variableName} = false;`);
> +		   } else {
> +		       emitter._add(`${variableName} = 0;`);
> +		   }
> +	       }

Single line conditionals.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:102
> +		   for (let i = 0; i < node.numElements.value; i++) {
> +		       emitter._zeroInitialize(node.elementType,
`${variableName}[${i}]`, false);
> +		   }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:244
> +	   } else {
> +	       this._add('}');
> +	   }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:263
> +	   } else {
> +	       throw new Error(`Couldn't determine type of ! expression
${node}'`);
> +	   }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:277
> +	   if
(this._canAvoidDereferenceBecauseDereferencingReferenceNode(node.ptr)) {
> +	       return this._doNotProduceReferenceFromReferenceNode(node.ptr);
> +	   } else {

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:370
> +		   if (anderRegex.test(node.nativeFuncInstance.name)) {
> +		       return emitter._handleFieldAccess(node);
> +		   } else if (node.nativeFuncInstance.name === "operator&[]") {
> +		       return emitter._handleIndexAccess(node);
> +		   }

Again.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:425
> +	   } else {
> +	       return this._nameMap.get(node.name);
> +	   }

Again.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:441
> +	   for (let expr of node.list) {
> +	       result = Node.visit(expr, this);
> +	   }

Again. I'll stop pointing them out now.