Bug 198580 – [WHLSL] Do not generate duplicate constructors/copy constructors in synthesizeConstructors

• Component: WebGPU
• Status: RESOLVED FIXED

[Saam Barati]

e.g:
void foo(thread int*) { }
void foo2(thread int*) { }
void foo3(thread int*) { }
void foo4(thread int*) { }
void foo5(thread int*) { }


We will end up with a bunch of constructors/copy constructors for "thread int*"

[Saam Barati]

This crashes the overload resolution in autoInitializeVariables.

[Saam Barati]

Created attachment 372083 [details]
WIP

contains code from OOB patch. Might be done besides just waiting for that patch to land.

[Saam Barati]

Created attachment 372131 [details]
patch

[EWS Watchlist]

Attachment 372131 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLTypeReference.h:116:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 372134 [details]
patch

fix style issue

[Robin Morisset]

Comment on attachment 372134 [details]
patch

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

r=me

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameResolver.cpp:93
>      newNameResolver.setCurrentFunctionDefinition(m_currentFunction);

These repeated copying of m_currentFunction and m_resolvingCalls make me think we should have a constructor of NameResolver that takes a reference to a parent NameResolver.
This constructor could easily then do the copying and could even take care of allocating the new NameContext.
This is not super urgent or important though, it would just be cleaner.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameResolver.cpp:225
> +    if (m_isResolvingCalls) {

This could be made a tad more readable by an early exit:
if (!m_isResolvingCalls)
  return;
and then the old code. 5 nested if expressions is getting a bit much.

[Saam Barati]

Created attachment 372312 [details]
patch for landing

[Saam Barati]

Comment on attachment 372134 [details]
patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameResolver.cpp:93
>>      newNameResolver.setCurrentFunctionDefinition(m_currentFunction);
> 
> These repeated copying of m_currentFunction and m_resolvingCalls make me think we should have a constructor of NameResolver that takes a reference to a parent NameResolver.
> This constructor could easily then do the copying and could even take care of allocating the new NameContext.
> This is not super urgent or important though, it would just be cleaner.

I will make this change.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameResolver.cpp:225
>> +    if (m_isResolvingCalls) {
> 
> This could be made a tad more readable by an early exit:
> if (!m_isResolvingCalls)
>   return;
> and then the old code. 5 nested if expressions is getting a bit much.

I agree it's not great, but I'm going to leave this as is, since we do things at the end of the if that we do if we do/don't take it.

[Saam Barati]

Created attachment 372348 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 372348 [details]
patch for landing

Rejecting attachment 372348 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 372348, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Robin Morisset found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/12509627

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/246550/webkit

[Radar WebKit Bug Importer]

<rdar://problem/51856246>