Bug 58109 – Some Handle<T> cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Some Handle<T> cleanup

[Geoffrey Garen]

Created attachment 88750 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 88750 [details]
Patch

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?

[Geoffrey Garen]

> > -/*
> > -    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.

Yeah, a bit odd. I chose the name because it was a good opposite to "Local<T>", which is local to a given scope. I'm not sure what would be better. Maybe "Root" or "Persistent"?

> 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"?

I just renamed both to set.

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

Yeah, maybe WeakHandle<T>, Weak<T>, or WeakGlobal<T> would be better.  I'll check with Oliver to see what he thinks.

[Geoffrey Garen]

Committed r83259: <http://trac.webkit.org/changeset/83259>