[webkit-reviews] review granted: [Bug 58109] Some Handle<T> cleanup : [Attachment 88750] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has granted Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 58109: Some Handle<T> cleanup
https://bugs.webkit.org/show_bug.cgi?id=58109

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

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88750&action=review

r=me, but see minor comments.

> Source/JavaScriptCore/collector/handles/Global.h:36
> -/*
> -    A Global is a persistent handle whose lifetime is not limited to any
given
> -    scope. Use Globals for data members and global variables.
> -*/
> -
> +// A Global is a persistent handle whose lifetime is not limited to any
given scope.

This is perhaps beyond the scope of this patch, but it seems a little odd for
something named Global to be used for data members.

> Source/JavaScriptCore/collector/handles/Global.h:94
>      void set(JSGlobalData& globalData, ExternalType value)
>      {
> -	   if (!value) {
> -	       clear();
> -	       return;
> -	   }
> -	   if (!this->slot())
> -	       this->setSlot(globalData.allocateGlobalHandle());
> -	   internalSet(value);
> +	   if (!slot())
> +	       setSlot(globalData.allocateGlobalHandle());
> +	   setWithWriteBarrier(value);
>      }

Looking at this, the distinction between set() and setWithWriteBarrier() seems
a little unclear. set() calls setWithWriteBarrier(), so isn't it equally "with
write barrier"? Perhaps there is a better way to draw the distinction between
the two functions.

> Source/JavaScriptCore/collector/handles/Local.h:98
> +template <typename T> inline void Local<T>::setWithSlotCheck(ExternalType
value)
> +{
> +    ASSERT(slot());
> +    *slot() = value;
> +}

Is the assert here the alluded-to "slot check"?

> Source/JavaScriptCore/runtime/WeakGCPtr.h:37
> +// A smart pointer that becomes 0 when the value it points to is garbage
collected.
> +template <typename T> class WeakGCPtr : public Handle<T> {

Maybe it would be appropriate to rename this to WeakHandle or something?