| Summary: | [WebGPU] Add support for the "is valid to use with" operation to WebGPU objects | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||
| Component: | WebGPU | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dino, djg, kkinnunen, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 238711 | ||||||||
| Bug Blocks: | 238720 | ||||||||
| Attachments: |
|
||||||||
|
Description
Myles C. Maxfield
2022-04-03 23:38:42 PDT
Created attachment 456535 [details]
Patch
Created attachment 456546 [details]
Patch
Comment on attachment 456546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456546&action=review also changes refcounts to thread safe (which is probably good) > Source/WebGPU/WebGPU/ObjectBase.h:40 > + ~ObjectBase(); Surprisingly ThreadSafeRefCounted has a footgun where the destructor is not a virtual. So written this way, Ref<ObjectBase> would never run the subclass destructors, which is a hard to find down the line if you ever end up using that. It's better to mark this virtual. If you don't intend to support Ref<ObjectBase>, then it's better to leave the threadsaferefcounted in the held classes. Comment on attachment 456546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456546&action=review >> Source/WebGPU/WebGPU/ObjectBase.h:40 >> + ~ObjectBase(); > > Surprisingly ThreadSafeRefCounted has a footgun where the destructor is not a virtual. > So written this way, Ref<ObjectBase> would never run the subclass destructors, which is a hard to find down the line if you ever end up using that. > It's better to mark this virtual. > > If you don't intend to support Ref<ObjectBase>, then it's better to leave the threadsaferefcounted in the held classes. Thinking about this more, I think there may actually be a better way of doing this. The whole point of this ObjectBase class is just to hold the isValidToUseWith() function, which is already templated. So, if it's already templated, it doesn't really need to actually exist in the base class. It can just be a free function and call .device() on the templated type. Doing it that way will get rid of this whole type hierarchy, and the problem you're describing here, as well as reducing total amount of code. Committed r292558 (249396@trunk): <https://commits.webkit.org/249396@trunk> Committed r292574 (?): <https://commits.webkit.org/r292574> (In reply to Myles C. Maxfield from comment #4) > [...] It > can just be a free function and call .device() on the templated type. Doing > it that way will get rid of this whole type hierarchy, and the problem > you're describing here, as well as reducing total amount of code. I like the sound of that! |