[webkit-reviews] review granted: [Bug 136602] XPCPtr should be converted into an all purpose smart pointer for os_objects : [Attachment 237742] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 136602: XPCPtr should be converted into an all purpose smart pointer for
os_objects
https://bugs.webkit.org/show_bug.cgi?id=136602

Attachment 237742: Patch
https://bugs.webkit.org/attachment.cgi?id=237742&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.