[webkit-reviews] review granted: [Bug 143919] Merge WeakMapData into JSWeakMap and JSWeakSet : [Attachment 318969] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 25 20:42:35 PDT 2017
Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 143919: Merge WeakMapData into JSWeakMap and JSWeakSet
https://bugs.webkit.org/show_bug.cgi?id=143919
Attachment 318969: Patch
https://bugs.webkit.org/attachment.cgi?id=318969&action=review
--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 318969
--> https://bugs.webkit.org/attachment.cgi?id=318969
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=318969&action=review
I’m not a big fan of the abbreviation “Impl” as in “WeakMapImpl”; I would have
used the word “Base” instead and called it “WeakMapBase”, I think.
> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:47
> +void WeakMapImpl::finishCreation(VM& vm)
> +{
> + Base::finishCreation(vm);
> +}
Do we really need this function?
> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:56
> + WeakMapImpl* thisObj = jsCast<WeakMapImpl*>(cell);
I would use auto* here since the type name is already there. I also would use
the word “object”, not the abbreviation “obj”. But these are changes to code
that was already here, I guess. And of course I would use a reference instead
of a pointer, but I think the people who normally work on JavaScriptCore
strongly reject this preference of mine.
> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:63
> + WeakMapImpl* thisObj = jsCast<WeakMapImpl*>(cell);
Ditto.
> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:95
> + auto iter = m_map.find(key);
> + if (iter == m_map.end())
> + return false;
> +
> + m_map.remove(iter);
> + return true;
This entire sequence of code does the same thing as:
return m_map.remove(key);
> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:118
> + RELEASE_ASSERT(m_liveKeyCount <= impl->m_map.size());
Very strange to use RELEASE_ASSERT here. There is no chance this assertion will
fail, given the way the loop above is written, so why include the code in
release builds? A normal ASSERT makes way more sense to me.
> Source/JavaScriptCore/runtime/WeakMapImpl.cpp:123
> + WeakMapImpl* impl = target();
I would call this map, or weakMap, not impl. Words preferred over
abbreviations.
> Source/JavaScriptCore/runtime/WeakMapImpl.h:47
> + typedef HashMap<JSObject*, WriteBarrier<Unknown>> MapType;
In new code we should us "using" rather than "typedef".
> Source/JavaScriptCore/runtime/WeakMapImpl.h:51
> + int size() const { return m_map.size(); }
The choice of "int" for the type here is peculiar. HashMap uses "unsigned".
> Source/JavaScriptCore/runtime/WeakMapImpl.h:58
> + void finishCreation(VM&);
Strange to override the public function in the base class with a protected
function. Maybe we should be inheriting protected instead of public?
> Source/JavaScriptCore/runtime/WeakMapImpl.h:65
> + DeadKeyCleaner(WeakMapImpl* impl)
> + {
> + ASSERT_UNUSED(impl, impl == target());
> + }
I think we should just assert this in the WeakMapImpl constructor after
constructing the DeadKeyCleaner. Seems like overkill to write this constructor
here in this header file just so we can put the assertion here.
> Source/JavaScriptCore/runtime/WeakMapImpl.h:70
> + WeakMapImpl* target()
> + {
> + return bitwise_cast<WeakMapImpl*>(bitwise_cast<char*>(this) -
OBJECT_OFFSETOF(WeakMapImpl, m_deadKeyCleaner));
> + }
I suggest putting this function body inside the cpp file rather than here in
the header. It can still be marked inline, but since it will only be called
inside the cpp file we should not have to put it here. We can still define the
function here.
More information about the webkit-reviews
mailing list