Summary: | Add move semantics to RefPtr | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||||||
Component: | Web Template Framework | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, ap, benjamin, buildbot, cmarcelo, commit-queue, darin, rniwa | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Mikhail Pozdnyakov
2013-04-23 06:32:20 PDT
Created attachment 199233 [details]
patch
Attachment 199233 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:84: Missing spaces around && [whitespace/operators] [3]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 199233 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=199233&action=review This is great, but I'd appreciate if you could upload a new patch so we can let the EWS bots chew at it - it's a pretty fundamental change and it could break the build in weird wais. > Source/WTF/wtf/RefPtr.h:47 > + ALWAYS_INLINE RefPtr(RefPtr&& o) : m_ptr(o.get()) { o.m_ptr = 0; } I think this would read better as RefPtr(RefPtr&& o) : m_ptr(o.release().leakRef()) { } > Source/WTF/wtf/RefPtr.h:48 > + template<typename U> RefPtr(RefPtr<U>&& o) : m_ptr(o.get()) { o.m_ptr = 0; } Same thing here. > Source/WTF/wtf/RefPtr.h:163 > + if (m_ptr != o.get()) { > + derefIfNotNull(m_ptr); > + m_ptr = o.get(); > + o.m_ptr = 0; > + } > + return *this; I think this could just be m_ptr = o.release(); > Source/WTF/wtf/RefPtr.h:172 > + if (m_ptr != o.get()) { > + derefIfNotNull(m_ptr); > + m_ptr = o.get(); > + o.m_ptr = 0; > + } Ditto. Created attachment 199236 [details]
patch v2
Updated due to comments from Anders. Thanks Anders!
Comment on attachment 199236 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=199236&action=review > Source/WTF/wtf/RefPtr.h:85 > + RefPtr& operator=(RefPtr&& o) { m_ptr = o.release(); } > + template<typename U> RefPtr& operator=(RefPtr<U>&& o) { m_ptr = o.release(); } These will cause a memory leak now, and return *this is gone. Let's change them back into what you had before; something like: if (m_ptr != o.m_ptr)) { derefIfNotNull(m_ptr); m_ptr = o.release().leakRef(); } return *this; (In reply to comment #5) > (From update of attachment 199236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199236&action=review > > > Source/WTF/wtf/RefPtr.h:85 > > + RefPtr& operator=(RefPtr&& o) { m_ptr = o.release(); } > > + template<typename U> RefPtr& operator=(RefPtr<U>&& o) { m_ptr = o.release(); } > > These will cause a memory leak now, and return *this is gone. omg! that's my absent-mindedness :( Created attachment 199244 [details]
patch v3
fixed assignment operators :/
Attachment 199244 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:86: Missing spaces around && [whitespace/operators] [3]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> That would allow us to obviate unnecessary reffing/ureffing when RefPtr is created or assigned from rvalue references.
Do we have examples of that in WebKit now? We are supposed to use PassRefPtr for poor man's move semantics.
To be clear, I certainly don't object to this change.
(In reply to comment #9) > > That would allow us to obviate unnecessary reffing/ureffing when RefPtr is created or assigned from rvalue references. > > Do we have examples of that in WebKit now? We are supposed to use PassRefPtr for poor man's move semantics. There is no explicit use of rvalue reference, but clang is smart enough to use it in some cases. Comment on attachment 199244 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=199244&action=review > Source/WTF/wtf/RefPtr.h:160 > + if (m_ptr != o.get()) { > + derefIfNotNull(m_ptr); > + m_ptr = o.release().leakRef(); > + } > + return *this; I don't understand why you do this. If you have RefPtr a = [foobar]; RefPtr b = a; a = std::move(b); "b" is unchanged, and the refcount is +2. This is counter-intuitive for move semantic. > Source/WTF/wtf/RefPtr.h:169 > + if (m_ptr != o.get()) { > + derefIfNotNull(m_ptr); > + m_ptr = o.release().leakRef(); > + } > + return *this; ditto Created attachment 199413 [details]
patch v4
Addresses Benjamin's concern.
Attachment 199413 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:86: Missing spaces around && [whitespace/operators] [3]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 199413 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=199413&action=review > Source/WTF/wtf/RefPtr.h:159 > + T* ptr = m_ptr; > + m_ptr = o.release().leakRef(); > + derefIfNotNull(ptr); > + return *this; With this, what if I do this? RefPtr a = [...]; a = std::move(a); (In reply to comment #14) > (From update of attachment 199413 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199413&action=review > > > Source/WTF/wtf/RefPtr.h:159 > > + T* ptr = m_ptr; > > + m_ptr = o.release().leakRef(); > > + derefIfNotNull(ptr); > > + return *this; > > With this, what if I do this? > > RefPtr a = [...]; > a = std::move(a); mm, wouldn't it mean that we do not care about 'a' anymore? so that looks like caller's fault. btw, in RetainPtr we have exactly same implementation: template<typename T> inline RetainPtr<T>& RetainPtr<T>::operator=(RetainPtr<T>&& o) { adoptCF(o.leakRef()); return *this; } and template<typename T> inline void RetainPtr<T>::adoptCF(PtrType optr) { PtrType ptr = m_ptr; m_ptr = optr; if (ptr) CFRelease(ptr); } (In reply to comment #11) > (From update of attachment 199244 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199244&action=review > > > Source/WTF/wtf/RefPtr.h:160 > > + if (m_ptr != o.get()) { > > + derefIfNotNull(m_ptr); > > + m_ptr = o.release().leakRef(); > > + } > > + return *this; > > I don't understand why you do this. My mistake, I think this check should just be: if (this != &o) { ... } (In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 199413 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=199413&action=review > > > > > Source/WTF/wtf/RefPtr.h:159 > > > + T* ptr = m_ptr; > > > + m_ptr = o.release().leakRef(); > > > + derefIfNotNull(ptr); > > > + return *this; > > > > With this, what if I do this? > > > > RefPtr a = [...]; > > a = std::move(a); > > mm, wouldn't it mean that we do not care about 'a' anymore? so that looks like caller's fault. > btw, in RetainPtr we have exactly same implementation: > template<typename T> inline RetainPtr<T>& RetainPtr<T>::operator=(RetainPtr<T>&& o) > { > adoptCF(o.leakRef()); > return *this; > } > > and > > template<typename T> inline void RetainPtr<T>::adoptCF(PtrType optr) > { > PtrType ptr = m_ptr; > m_ptr = optr; > if (ptr) > CFRelease(ptr); > } yeah, with RetainPtr it's different as it\s releasing m_ptr which is 0 after leakPtr() call. Created attachment 199733 [details]
patch v5
Fixed '=' operator.
Attachment 199733 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:86: Missing spaces around && [whitespace/operators] [3]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 199733 [details] patch v5 Attachment 199733 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/144200 Comment on attachment 199733 [details] patch v5 Attachment 199733 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/175534 Comment on attachment 199733 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=199733&action=review > Source/WTF/wtf/RefPtr.h:168 > + if (this != &o) { > + derefIfNotNull(m_ptr); > + m_ptr = o.release().leakRef(); > + } It’s difficult to get this right; often this and &o are not objects of the same type, so we can’t compare them. I suggest we instead consider the canonical operator= implementation: RefPtr<T> ptr = o; swap(ptr); return *this; We could use this for both operator= overloads. As long as it compiles to efficient-enough code. Comment on attachment 199733 [details] patch v5 Attachment 199733 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/19238 (In reply to comment #22) . > > I suggest we instead consider the canonical operator= implementation: > > RefPtr<T> ptr = o; > swap(ptr); > return *this; I like that. The first row should be RefPtr<T> ptr = std::move(o); (In reply to comment #24) > (In reply to comment #22) > . > > > > I suggest we instead consider the canonical operator= implementation: > > > > RefPtr<T> ptr = o; > > swap(ptr); > > return *this; > > I like that. The first row should be > > RefPtr<T> ptr = std::move(o); That's great! but how about just RefPtr<T>(std::move(o)).swap(*this); return *this; Created attachment 199804 [details]
patch v6
Canonical '=' operator implementation.
Attachment 199804 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/RefPtr.h']" exit_code: 1
Source/WTF/wtf/RefPtr.h:87: Missing spaces around && [whitespace/operators] [3]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 199804 [details] patch v6 (In reply to comment #25) > That's great! but how about just > > RefPtr<T>(std::move(o)).swap(*this); > return *this; I think it's easier to read with an explicit temporary. Please make that change, r=me. Committed r149184: <http://trac.webkit.org/changeset/149184> |