Summary: | XPCPtr should be converted into an all purpose smart pointer for os_objects | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | benjamin, cmarcelo, commit-queue, mitz | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Sam Weinig
2014-09-06 12:40:32 PDT
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. |