RESOLVED WONTFIX 116204
Add NeverNullRefPtr, a ref-pointer that always points to a value
https://bugs.webkit.org/show_bug.cgi?id=116204
Summary Add NeverNullRefPtr, a ref-pointer that always points to a value
Benjamin Poulain
Reported 2013-05-15 21:31:26 PDT
Add NeverNullRefPtr, a ref-pointer that always points to a value
Attachments
Patch (21.79 KB, patch)
2013-05-15 21:36 PDT, Benjamin Poulain
no flags
Patch (21.75 KB, patch)
2013-05-15 21:48 PDT, Benjamin Poulain
no flags
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (800.72 KB, application/zip)
2013-05-16 11:07 PDT, Build Bot
no flags
Patch (36.45 KB, patch)
2013-07-31 21:23 PDT, Benjamin Poulain
no flags
Benjamin Poulain
Comment 1 2013-05-15 21:36:55 PDT
Benjamin Poulain
Comment 2 2013-05-15 21:44:19 PDT
I tried to keep it simple at first. Short term I would like to: -Add hash map support. -Add NeverNullPtr. Abstracting a pointer that cannot be null. -Add NeverNullOwnPtr. Maybe long term (at least after things get quieter): -Rename RefPtr to NullableRefPtr -Rename NeverNullFooBar to FooBar Some concerns: -Going through JSC, many many more RefPtr are nullable than I expected. NeverNullRefPtr will be less useful than I hoped. It seems most RefPtr are non-null most of the time, but not all the time. -I am afraid all the if (!null) CRASH() will bloat our binary a little. -The PassRefPtr are tricky.
Benjamin Poulain
Comment 3 2013-05-15 21:48:25 PDT
Mikhail Pozdnyakov
Comment 4 2013-05-16 00:53:13 PDT
Comment on attachment 201919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201919&action=review > Source/WTF/wtf/NeverNullRefPtr.h:34 > + NeverNullRefPtr(NeverNullPtrFlag, T*); is it supposed to be used only by 'createRefCounted'? if so maybe it's worth making it private to avoid possible misuse? > Source/WTF/wtf/NeverNullRefPtr.h:39 > + template<typename U> NeverNullRefPtr(const PassRefPtr<U>&); maybe move semantics is also worth being added. (guess template<typename U> PassRefPtr(const NeverNullRefPtr<U>&); is not added on purpose, but we might need a way of cheap NeverNullRefPtr passing) > Source/WTF/wtf/NeverNullRefPtr.h:107 > + this->swap(temp); why "this->" is needed? > Source/WTF/wtf/NeverNullRefPtr.h:199 > +template<typename T, typename Arg1> inline NeverNullRefPtr<T> createRefCounted(Arg1 arg1) const Arg1& ?
Mikhail Pozdnyakov
Comment 5 2013-05-16 01:10:06 PDT
Comment on attachment 201919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201919&action=review > Source/WTF/wtf/NeverNullRefPtr.h:66 > + m_ptr->ref(); shouldn't it be an adopting constructor?
Benjamin Poulain
Comment 6 2013-05-16 01:46:15 PDT
Comment on attachment 201919 [details] Patch The more I think about it, the less I am convinced this will scale while promoting good practices. I need to consider alternatives.
Build Bot
Comment 7 2013-05-16 11:07:00 PDT
Comment on attachment 201919 [details] Patch Attachment 201919 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/491005 New failing tests: fast/events/mouse-cursor-image-set.html
Build Bot
Comment 8 2013-05-16 11:07:03 PDT
Created attachment 201973 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Andreas Kling
Comment 9 2013-07-29 14:10:31 PDT
Comment on attachment 201919 [details] Patch I like this class. I would put it to use right away in WebCore::DataRef.
Benjamin Poulain
Comment 10 2013-07-31 21:23:40 PDT
Andreas Kling
Comment 11 2013-08-30 09:53:38 PDT
Comment on attachment 207896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207896&action=review > Source/WTF/wtf/NeverNullRefPtr.h:95 > +template<typename T> template<typename U> > +ALWAYS_INLINE NeverNullRefPtr<T>::NeverNullRefPtr(U* pointer) > + : m_ptr(pointer) > +{ > + if (UNLIKELY(!m_ptr)) > + CRASH(); > + pointer->ref(); > +} We should design this class to take a U& instead of a U*. Then we won't need the null-check branch.
Andreas Kling
Comment 12 2013-08-31 22:34:00 PDT
I spun off bug 120570 to experiment with a smaller version of this same thing. Maybe I should have kept it here. Oh well.
Brent Fulgham
Comment 13 2014-01-21 11:39:44 PST
Is this patch still relevant, in light of Bug 120570?
Benjamin Poulain
Comment 14 2014-01-21 11:46:01 PST
Comment on attachment 207896 [details] Patch Let's nuke this.
Note You need to log in before you can comment on or make changes to this bug.