[webkit-reviews] review granted: [Bug 70336] Add finalizer to JSObject : [Attachment 111683] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 70336: Add finalizer to JSObject
https://bugs.webkit.org/show_bug.cgi?id=70336

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

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


> Source/JavaScriptCore/ChangeLog:14
> +	   (JSC::JSCell::~JSCell): Remove the debugging information due to a
conflict with 
> +	   future changes and the fact that we no longer always call the
destructor, making 
> +	   the debugging information provided less useful.

I’m not sure why m_structure.clear() is called “the debugging information”.

> Source/JavaScriptCore/runtime/JSCell.h:-182
> -#if ENABLE(GC_VALIDATION)
> -	   m_structure.clear();
> -#endif

Don’t you also have to remove the code that checks this?

> Source/JavaScriptCore/runtime/JSObject.cpp:675
> +	   Heap::heap(this)->addFinalizer(this, &JSObject::finalize);

You should not need JSObject:: here. Just finalize should work.