RESOLVED FIXED 58109
Some Handle<T> cleanup
https://bugs.webkit.org/show_bug.cgi?id=58109
Summary Some Handle<T> cleanup
Geoffrey Garen
Reported 2011-04-07 18:16:31 PDT
Some Handle<T> cleanup
Attachments
Patch (31.52 KB, patch)
2011-04-07 18:30 PDT, Geoffrey Garen
mjs: review+
Geoffrey Garen
Comment 1 2011-04-07 18:30:25 PDT
Maciej Stachowiak
Comment 2 2011-04-07 22:06:24 PDT
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
Comment 3 2011-04-07 23:08:17 PDT
> > -/* > > - 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
Comment 4 2011-04-07 23:34:54 PDT
Note You need to log in before you can comment on or make changes to this bug.