[webkit-reviews] review denied: [Bug 31639] Add asserts to RefCounted to make sure ref/deref happens on the right thread. : [Attachment 44201] Proposed patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Dec 4 10:13:52 PST 2009


David Levin <levin at chromium.org> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 31639: Add asserts to RefCounted to make sure ref/deref happens on the
right thread.
https://bugs.webkit.org/show_bug.cgi?id=31639

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

------- Additional Comments from David Levin <levin at chromium.org>
> diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
> +	   (WTF::ThreadVerifier::activate): Activates hecks. Called when ref
count becomes above 2.

typo: hecks

> diff --git a/JavaScriptCore/runtime/Structure.cpp
b/JavaScriptCore/runtime/Structure.cpp
> +    // Use of addressOfCount() in this calss prevents thread verification in
RefCounted

typo: calss

> +    // since it modifies refcount via other means then ref()/deref().

s/then/than/

> diff --git a/JavaScriptCore/wtf/RefCounted.h
b/JavaScriptCore/wtf/RefCounted.h
> +#ifndef NDEBUG
> +class ThreadVerifier {

This seems like it should be in its own file. (As a side benefit, the whole
file can be ifndef NDEBUG include the wtf/Threading.h include.)

> +#endif

#endif // NDEBUG
Since this is a long ifdef.


> +#ifndef NDEBUG
> +    void disableThreadVerification()
> +    {

Personally, I'd prefer moving the ifndef inside of the method. This would still
make the call a no-op when NDEBUG is defined, but every call site wouldn't need
a ifndef NDEBUG around it.

> +	   m_threadVerifier.disableThreadVerification();
> +    }
> +#endif
> +


More information about the webkit-reviews mailing list