RESOLVED FIXED 238719
[WebGPU] Add support for the "is valid to use with" operation to WebGPU objects
https://bugs.webkit.org/show_bug.cgi?id=238719
Summary [WebGPU] Add support for the "is valid to use with" operation to WebGPU objects
Myles C. Maxfield
Reported 2022-04-03 23:38:42 PDT
.
Attachments
Patch (37.66 KB, patch)
2022-04-03 23:44 PDT, Myles C. Maxfield
no flags
Patch (56.92 KB, patch)
2022-04-04 01:05 PDT, Myles C. Maxfield
kkinnunen: review+
Myles C. Maxfield
Comment 1 2022-04-03 23:44:20 PDT
Myles C. Maxfield
Comment 2 2022-04-04 01:05:24 PDT
Kimmo Kinnunen
Comment 3 2022-04-07 02:57:39 PDT
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.
Myles C. Maxfield
Comment 4 2022-04-07 12:18:31 PDT
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.
Myles C. Maxfield
Comment 5 2022-04-07 13:23:16 PDT
Radar WebKit Bug Importer
Comment 6 2022-04-07 13:24:24 PDT
Myles C. Maxfield
Comment 7 2022-04-07 15:50:28 PDT
Dan Glastonbury
Comment 8 2022-04-07 18:58:30 PDT
(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!
Note You need to log in before you can comment on or make changes to this bug.