Bug 136602 – XPCPtr should be converted into an all purpose smart pointer for os_objects

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

XPCPtr should be converted into an all purpose smart pointer for os_objects

[Sam Weinig]

Created attachment 237742 [details]
Patch

[Darin Adler]

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.

[Sam Weinig]

(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.

[Sam Weinig]

Committed r173367: <http://trac.webkit.org/changeset/173367>

[Sam Weinig]

Feedback is being address in bug 136613.