[webkit-reviews] review denied: [Bug 68429] Use WTF::ThreadSpecific instead of pthread_key_t in JSC::MachineThreads : [Attachment 128011] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 68429: Use WTF::ThreadSpecific instead of pthread_key_t in
JSC::MachineThreads
https://bugs.webkit.org/show_bug.cgi?id=68429

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=128011&action=review


> Source/JavaScriptCore/wtf/ThreadSpecific.h:170
> +class ThreadSpecificKeyValue;

"KeyValue" is a very confusing name phrase. Our typical style is to use the
word "platform": "PlatformThreadSpecificKey".

> Source/JavaScriptCore/wtf/ThreadSpecific.h:176
> +void ThreadSpecificKeyCreate(ThreadSpecificKey*, void (*)(void *));
> +void ThreadSpecificKeyDelete(ThreadSpecificKey);
> +void ThreadSpecificSet(ThreadSpecificKey, void*);
> +void* ThreadSpecificGet(ThreadSpecificKey);

Capitalization here doesn't match WebKit style.

> Source/JavaScriptCore/wtf/ThreadSpecificWin.cpp:60
> +	   while (*next != this) {
> +	       ASSERT(*next);
> +	       next = &(*next)->m_next;
> +	   }
> +	   *next = (*next)->m_next;

Linear search is so 1960s. Please use a doubly linked list with O(1) removal
time.

> Source/JavaScriptCore/wtf/ThreadSpecificWin.cpp:84
> +    static ThreadSpecificKeyValue* m_first;

WebKit style is to use the "s_" prefix for static data. "m_" means "member"
data. Please don't call static data member data.