[webkit-reviews] review granted: [Bug 85119] Only allow non-null pointers in the WeakSet : [Attachment 139326] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 85119: Only allow non-null pointers in the WeakSet
https://bugs.webkit.org/show_bug.cgi?id=85119

Attachment 139326: Patch
https://bugs.webkit.org/attachment.cgi?id=139326&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=139326&action=review


> Source/JavaScriptCore/heap/PassWeak.h:127
>  template<typename T> inline PassWeak<T>::PassWeak(JSGlobalData& globalData,
typename PassWeak<T>::GetType getType, WeakHandleOwner* weakOwner, void*
context)
> -    : m_impl(globalData.heap.weakSet()->allocate(getType, weakOwner,
context))
> +    : m_impl(0)
>  {
> +    if (!getType)
> +	   return;
> +    m_impl = globalData.heap.weakSet()->allocate(getType, weakOwner,
context);
>  }

As with your prior WebCore patch, I think there are two other ways you could do
this:

1) A helper function that returns 0 if getType is 0.
2) A trinary expression getType ? xxx : 0.

The “initialize to 0 and then use an early exit” doesn’t seem quite as good as
those.

> Source/JavaScriptCore/heap/WeakBlock.cpp:109
>	   const JSValue& jsValue = weakImpl->jsValue();
> -	   if (!jsValue || !jsValue.isCell())
> -	       continue;
> -
>	   JSCell* jsCell = jsValue.asCell();
>	   if (Heap::isMarked(jsCell))
>	       continue;

Why bother with all these local variables? I like this:

    if (Heap::isMarked(weakImpl->jsValue().asCell()))

> Source/JavaScriptCore/heap/WeakBlock.cpp:136
>	   const JSValue& jsValue = weakImpl->jsValue();
> -	   if (!jsValue || !jsValue.isCell())
> -	       continue;
> -
>	   JSCell* jsCell = jsValue.asCell();
>	   if (Heap::isMarked(jsCell))
>	       continue;

Why bother with all these local variables? I like this:

    if (Heap::isMarked(weakImpl->jsValue().asCell()))