WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(21.75 KB, patch)
2013-05-15 21:48 PDT
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(36.45 KB, patch)
2013-07-31 21:23 PDT
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Benjamin Poulain
Comment 1
2013-05-15 21:36:55 PDT
Created
attachment 201918
[details]
Patch
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
Created
attachment 201919
[details]
Patch
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
Created
attachment 207896
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug