XPCPtr should be converted into an all purpose smart pointer for os_objects
Created attachment 237742 [details] Patch
Comment on attachment 237742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237742&action=review Generally I’d like to see this more similar to RefPtr, maybe even sharing code somehow. If we made ref/deref free functions instead of member functions, I think we’d be able to share a single class template. But why don’t I feel that way about RetainPtr? Too many special features? Hard to say. > Source/WTF/wtf/OSObjectPtr.h:89 > + OSObjectPtr(AdoptOSObject, T ptr) For PassRefPtr we made this private and made adoptRef a friend. Might want that here too. > Source/WTF/wtf/OSObjectPtr.h:118 > + OSObjectPtr& operator=(const OSObjectPtr& other) I like the swap style we use in RefPtr better. Also, missing the move assignment operator. > Source/WTF/wtf/OSObjectPtr.h:124 > T ptr = m_ptr; No need for this local variable. It’s fine to release before assigning m_ptr, since we have already retained optr. That’s what we do in the nullptr specialization below, in fact. > Source/WTF/wtf/OSObjectPtr.h:133 > + OSObjectPtr& operator=(std::nullptr_t) With all the inlining, I’m surprised we need a specialization for nullptr.
(In reply to comment #2) > (From update of attachment 237742 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237742&action=review > > Generally I’d like to see this more similar to RefPtr, maybe even sharing code somehow. If we made ref/deref free functions instead of member functions, I think we’d be able to share a single class template. But why don’t I feel that way about RetainPtr? Too many special features? Hard to say. > > > Source/WTF/wtf/OSObjectPtr.h:89 > > + OSObjectPtr(AdoptOSObject, T ptr) > > For PassRefPtr we made this private and made adoptRef a friend. Might want that here too. > > > Source/WTF/wtf/OSObjectPtr.h:118 > > + OSObjectPtr& operator=(const OSObjectPtr& other) > > I like the swap style we use in RefPtr better. > > Also, missing the move assignment operator. > > > Source/WTF/wtf/OSObjectPtr.h:124 > > T ptr = m_ptr; > > No need for this local variable. It’s fine to release before assigning m_ptr, since we have already retained optr. That’s what we do in the nullptr specialization below, in fact. > > > Source/WTF/wtf/OSObjectPtr.h:133 > > + OSObjectPtr& operator=(std::nullptr_t) > > With all the inlining, I’m surprised we need a specialization for nullptr. Thanks for the review. Given that I am mostly just moving this class, I am going to make these changes in a follow up.
Committed r173367: <http://trac.webkit.org/changeset/173367>
Feedback is being address in bug 136613.