Bug 116204 – Add NeverNullRefPtr, a ref-pointer that always points to a value

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Benjamin Poulain]

Add NeverNullRefPtr, a ref-pointer that always points to a value

[Benjamin Poulain]

Created attachment 201918 [details]
Patch

[Benjamin Poulain]

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]

Created attachment 201919 [details]
Patch

[Mikhail Pozdnyakov]

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 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 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 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]

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 on attachment 201919 [details]
Patch

I like this class. I would put it to use right away in WebCore::DataRef.

[Benjamin Poulain]

Created attachment 207896 [details]
Patch

[Andreas Kling]

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]

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]

Is this patch still relevant, in light of Bug 120570?

[Benjamin Poulain]

Comment on attachment 207896 [details]
Patch

Let's nuke this.