| Summary: | [WebGPU] Abide by the WebKit Style Guide | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin, dino, djg, kkinnunen, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Myles C. Maxfield
2022-03-03 10:27:29 PST
Created attachment 453758 [details]
Patch
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. 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." 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++? 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. 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. 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. 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. Created attachment 453767 [details]
Patch
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]. |