[webkit-reviews] review granted: [Bug 135358] [ftlopt] Create a more generic way for VMEntryScope to notify those interested that it will be destroyed : [Attachment 236068] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 135358: [ftlopt] Create a more generic way for VMEntryScope to notify those
interested that it will be destroyed
https://bugs.webkit.org/show_bug.cgi?id=135358

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

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


> Source/JavaScriptCore/runtime/VMEntryScope.cpp:59
> +    m_allEntryScopeDidPopListeners.set(key, listener);

Probably should be add instead of set; set is a slower version that knows how
to override values already in the map, and I think the intent here is not to do
that.

Also, I think we should assert that they key isn’t already in there.

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:74
> +    auto iter = m_allEntryScopeDidPopListeners.begin();
> +    auto end = m_allEntryScopeDidPopListeners.end();
> +    for ( ; iter != end; ++iter) {
> +	   EntryScopeDidPopListener listener = iter->value;
> +	   listener(&m_vm, m_globalObject);
>      }

The typical way of writing this kind of thing in new WebKit code is:

    for (auto& listener : m_allEntryScopeDidPopListeners.values())
	listener(&m_vm, m_globalObject);

I think that’s easier to read than the 6-line version.

> Source/JavaScriptCore/runtime/VMEntryScope.h:46
> +    typedef std::function<void (VM*, JSGlobalObject*)>
EntryScopeDidPopListener;

Argument should be VM& instead of VM*.

> Source/JavaScriptCore/runtime/VMEntryScope.h:52
> +    HashMap<void*, EntryScopeDidPopListener> m_allEntryScopeDidPopListeners;


Is void* really the best type for these keys?

Why does this need to be a map? Can’t it just be a Vector? Do we ever remove or
overwrite any of these?