Prohibit the use of OwnPtr<T*>
Created attachment 91129 [details] Patch
There may be some better way to implement this, but I think it would be an excellent idea to prevent somebody accidentally using OwnPtr<T*> when they intended OwnPtr<T>. I can't think of any usage for the former. (Also, I'm a little unsure of who to CC on this bug, so please add others.)
Attachment 91129 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8509701
Attachment 91129 [details] did not build on win: Build output: http://queues.webkit.org/results/8508728
Attachment 91129 [details] did not build on mac: Build output: http://queues.webkit.org/results/8505948
Created attachment 91144 [details] Patch
Let's try this again now that the blocking bugfix has landed.
Attachment 91144 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8509721
Comment on attachment 91144 [details] Patch Oops. Need to touch up DRT as well.
Attachment 91129 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8510701
Attachment 91144 [details] did not build on win: Build output: http://queues.webkit.org/results/8509734
Attachment 91144 [details] did not build on mac: Build output: http://queues.webkit.org/results/8508780
Created attachment 91177 [details] Patch
Third time's a charm for fixing all the build errors.
Comment on attachment 91177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91177&action=review r=me > Source/JavaScriptCore/wtf/OwnPtr.h:179 > + template<typename T> class OwnPtr<T*> { > + // Prevent usage as a real OwnPtr by not defining any functions. > + }; > +#endif I believe you can do even better here by just using a declaration with no definition.
(In reply to comment #15) > (From update of attachment 91177 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91177&action=review > > r=me Thanks for all the reviews today. :) > > Source/JavaScriptCore/wtf/OwnPtr.h:179 > > + template<typename T> class OwnPtr<T*> { > > + // Prevent usage as a real OwnPtr by not defining any functions. > > + }; > > +#endif > > I believe you can do even better here by just using a declaration with no definition. That would be cleaner. I'll do that before landing.
Attachment 91177 [details] did not build on win: Build output: http://queues.webkit.org/results/8507775
(In reply to comment #16) > (In reply to comment #15) > > > Source/JavaScriptCore/wtf/OwnPtr.h:179 > > > + template<typename T> class OwnPtr<T*> { > > > + // Prevent usage as a real OwnPtr by not defining any functions. > > > + }; > > > +#endif > > > > I believe you can do even better here by just using a declaration with no definition. > > That would be cleaner. I'll do that before landing. After some study, I think an empty class rather than an incomplete declaration is better. If the class is incomplete, then it's harder to #ifdef out this error because every class that is-a or has-a class with an OwnPtr<T*> needs the ALLOW_RAW_POINTER_IN_OWNPTR define. Hopefully that should go away soon, but in the short term MediaPlayer.h is used in a number of places. Also, the error message points to OwnPtr.h in the former case where somebody might see the comment, but just says the slightly more obscure "incomplete declaration" in the latter.
Attachment 91144 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8510781
Oh, yuck. It looks like the Windows port is set up to handle OwnPtr<HBITMAP> (which is a void*), but has a deleteOwnPtr(HBITMAP) function which will properly clean up its resources. So, I can't disallow all OwnPtr<T*>, just all except any except the 10 or so port-specific pointer types listed in OwnPtrCommon.h. I'll have to take another look at this patch to account for that while still disallowing the general case.
Comment on attachment 91177 [details] Patch Clearing review flag because we need to rework this for Windows’ sake. Best possibility would probably be to change Windows to use to something other than OwnPtr. It’s not safe to have it using OwnPtr with void*!
(In reply to comment #21) > (From update of attachment 91177 [details]) > Clearing review flag because we need to rework this for Windows’ sake. Best possibility would probably be to change Windows to use to something other than OwnPtr. It’s not safe to have it using OwnPtr with void*! Ack. I thought I had closed this bug. It turns out that I was just mistaken. All the Windows-specific versions of OwnPtr like OwnPtr<HBITMAP> have their own specialized delete function which gets called. And thanks to template magic, for the general case of T, it turns out that OwnPtr<T*> is identical to OwnPtr<T> in terms of what gets stored in the OwnPtr and which delete function gets called.