Bug 237437 – [WebGPU] Abide by the WebKit Style Guide

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[WebGPU] Abide by the WebKit Style Guide

[Myles C. Maxfield]

Created attachment 453758 [details]
Patch

[Darin Adler]

Comment on attachment 453758 [details]
Patch

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

> Source/WebGPU/ChangeLog:9
> +        1. "Donât add explicit initializations to nil"

Wait, what? What is that guideline?

Under ARC things are initialized without doing it explicitly, but with manual retain/release I am pretty sure you need to do it!

> Source/WebGPU/WebGPU/BindGroup.h:55
> -    id <MTLBuffer> m_vertexArgumentBuffer { nil };
> -    id <MTLBuffer> m_fragmentArgumentBuffer { nil };
> -    id <MTLBuffer> m_computeArgumentBuffer { nil };
> +    id<MTLBuffer> m_vertexArgumentBuffer;
> +    id<MTLBuffer> m_fragmentArgumentBuffer;
> +    id<MTLBuffer> m_computeArgumentBuffer;

Removing the { nil } here does not seem right.

[Myles C. Maxfield]

Comment on attachment 453758 [details]
Patch

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

>> Source/WebGPU/ChangeLog:9
>> +        1. "Donât add explicit initializations to nil"
> 
> Wait, what? What is that guideline?
> 
> Under ARC things are initialized without doing it explicitly, but with manual retain/release I am pretty sure you need to do it!

https://webkit.org/code-style-guidelines/#zero-objc-variables

"In Objective-C, instance variables are initialized to zero automatically. Don’t add explicit initializations to nil or NO in an init method."

[Myles C. Maxfield]

Comment on attachment 453758 [details]
Patch

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

>>> Source/WebGPU/ChangeLog:9
>>> +        1. "Donât add explicit initializations to nil"
>> 
>> Wait, what? What is that guideline?
>> 
>> Under ARC things are initialized without doing it explicitly, but with manual retain/release I am pretty sure you need to do it!
> 
> https://webkit.org/code-style-guidelines/#zero-objc-variables
> 
> "In Objective-C, instance variables are initialized to zero automatically. Don’t add explicit initializations to nil or NO in an init method."

Oh, maybe that's just for ivars and not regular variables?

And maybe doesn't apply in Objective-C++?

[Darin Adler]

Comment on attachment 453758 [details]
Patch

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

>>>> Source/WebGPU/ChangeLog:9
>>>> +        1. "Donât add explicit initializations to nil"
>>> 
>>> Wait, what? What is that guideline?
>>> 
>>> Under ARC things are initialized without doing it explicitly, but with manual retain/release I am pretty sure you need to do it!
>> 
>> https://webkit.org/code-style-guidelines/#zero-objc-variables
>> 
>> "In Objective-C, instance variables are initialized to zero automatically. Don’t add explicit initializations to nil or NO in an init method."
> 
> Oh, maybe that's just for ivars and not regular variables?
> 
> And maybe doesn't apply in Objective-C++?

Yes, just instance variables. It does apply in Objective-C++, but structure members and C++ class members are not instance variables.

[Myles C. Maxfield]

Comment on attachment 453758 [details]
Patch

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

>>>>> Source/WebGPU/ChangeLog:9
>>>>> +        1. "Donât add explicit initializations to nil"
>>>> 
>>>> Wait, what? What is that guideline?
>>>> 
>>>> Under ARC things are initialized without doing it explicitly, but with manual retain/release I am pretty sure you need to do it!
>>> 
>>> https://webkit.org/code-style-guidelines/#zero-objc-variables
>>> 
>>> "In Objective-C, instance variables are initialized to zero automatically. Don’t add explicit initializations to nil or NO in an init method."
>> 
>> Oh, maybe that's just for ivars and not regular variables?
>> 
>> And maybe doesn't apply in Objective-C++?
> 
> Yes, just instance variables. It does apply in Objective-C++, but structure members and C++ class members are not instance variables.

Looking at the disassembly, it looks like it does zero-out uninitialized Obj-C objects in C++ classes. If I add a new uninitialized Obj-C member to the class, then the C++ constructor gains an extra instruction:

    0x1014c53d4 <+84>:  movq   $0x0, 0x10(%rax)

Either way, I'm happy to be defensive and explicitly zero-out these members.

[Darin Adler]

Comment on attachment 453758 [details]
Patch

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

>>>>>> Source/WebGPU/ChangeLog:9
>>>>>> +        1. "Donât add explicit initializations to nil"
>>>>> 
>>>>> Wait, what? What is that guideline?
>>>>> 
>>>>> Under ARC things are initialized without doing it explicitly, but with manual retain/release I am pretty sure you need to do it!
>>>> 
>>>> https://webkit.org/code-style-guidelines/#zero-objc-variables
>>>> 
>>>> "In Objective-C, instance variables are initialized to zero automatically. Don’t add explicit initializations to nil or NO in an init method."
>>> 
>>> Oh, maybe that's just for ivars and not regular variables?
>>> 
>>> And maybe doesn't apply in Objective-C++?
>> 
>> Yes, just instance variables. It does apply in Objective-C++, but structure members and C++ class members are not instance variables.
> 
> Looking at the disassembly, it looks like it does zero-out uninitialized Obj-C objects in C++ classes. If I add a new uninitialized Obj-C member to the class, then the C++ constructor gains an extra instruction:
> 
>     0x1014c53d4 <+84>:  movq   $0x0, 0x10(%rax)
> 
> Either way, I'm happy to be defensive and explicitly zero-out these members.

If so, this is a change from the ARC transition, although maybe it does affect non-ARC files perhaps due to a compiler option we were passing. This was not the case when we started the WebKit project; we need to double check it to be sure. Not sure that single experiment is enough. I welcome the change to remove the unneeded nil if it’s truly unneeded now.

[Darin Adler]

Comment on attachment 453758 [details]
Patch

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

>>>>>>> Source/WebGPU/ChangeLog:9
>>>>>>> +        1. "Donât add explicit initializations to nil"
>>>>>> 
>>>>>> Wait, what? What is that guideline?
>>>>>> 
>>>>>> Under ARC things are initialized without doing it explicitly, but with manual retain/release I am pretty sure you need to do it!
>>>>> 
>>>>> https://webkit.org/code-style-guidelines/#zero-objc-variables
>>>>> 
>>>>> "In Objective-C, instance variables are initialized to zero automatically. Don’t add explicit initializations to nil or NO in an init method."
>>>> 
>>>> Oh, maybe that's just for ivars and not regular variables?
>>>> 
>>>> And maybe doesn't apply in Objective-C++?
>>> 
>>> Yes, just instance variables. It does apply in Objective-C++, but structure members and C++ class members are not instance variables.
>> 
>> Looking at the disassembly, it looks like it does zero-out uninitialized Obj-C objects in C++ classes. If I add a new uninitialized Obj-C member to the class, then the C++ constructor gains an extra instruction:
>> 
>>     0x1014c53d4 <+84>:  movq   $0x0, 0x10(%rax)
>> 
>> Either way, I'm happy to be defensive and explicitly zero-out these members.
> 
> If so, this is a change from the ARC transition, although maybe it does affect non-ARC files perhaps due to a compiler option we were passing. This was not the case when we started the WebKit project; we need to double check it to be sure. Not sure that single experiment is enough. I welcome the change to remove the unneeded nil if it’s truly unneeded now.

The style guide is absolutely *not* talking about this, though.

[Myles C. Maxfield]

Created attachment 453767 [details]
Patch

[EWS]

Committed r290791 (248031@main): <https://commits.webkit.org/248031@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453767 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89772856>