[webkit-reviews] review granted: [Bug 207715] [JSC] JITThunk should be HashSet<Weak<NativeExecutable>> with appropriate GC weakness handling : [Attachment 390910] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 207715: [JSC] JITThunk should be HashSet<Weak<NativeExecutable>> with
appropriate GC weakness handling
https://bugs.webkit.org/show_bug.cgi?id=207715

Attachment 390910: Patch

https://bugs.webkit.org/attachment.cgi?id=390910&action=review




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 390910
  --> https://bugs.webkit.org/attachment.cgi?id=390910
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390910&action=review

> Source/JavaScriptCore/jit/JITThunks.cpp:69
> +    if (&a == &b)
> +	   return true;

Is this an important optimization?

> Source/JavaScriptCore/jit/JITThunks.cpp:91
> +    auto* aImpl = a.unsafeImpl();
> +    ASSERT(aImpl);
> +    ASSERT(aImpl->state() != WeakImpl::State::Deallocated);
> +    return equal(*static_cast<NativeExecutable*>(aImpl->jsValue().asCell()),
*bExecutable);

I think that this sequence should be a helper function. Going from a
Weak<NativeExecutable> to a NativeExecutable& with all the assertions. Instead
it’s repeated three times. Twice above and once here.