[webkit-reviews] review denied: [Bug 22614] Need to add Win32 implementation of ThreadSpecific. : [Attachment 26105] Proposed Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 22614: Need to add Win32 implementation of ThreadSpecific.
https://bugs.webkit.org/show_bug.cgi?id=22614

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Are you suggesting the code for use on all Win32 ports, or only on those that
don't use pthreads already? I can't tell for sure from the patch - I may be
mistaken, but it seems that ifdefs are still confused a little.


> +	   * JavaScriptCore.xcodeproj/project.pbxproj:

All code in ThreadSpecific.cpp is Windows-only, so it should be called
ThreadSpecificWin.cpp, and it shouldn't be in this Mac-specific project file.

> + * Copyright (C) 2008 Apple Inc. All rights reserved.

I don't see any Apple code in this file, so you needn't assign copyright to
Apple.

> +
> +#include "ThreadSpecific.h"

All cpp files must include config.h first.

> +#if PLATFORM(WIN_OS)

In ThreadSpecificWin.cpp, there will be no need for the ifdef.

> +void ThreadSpecificThreadExit() {

The brace should go on next line.

> +    ThreadSpecific<int>::ThreadExit();

I think that ThreadExit() function needn't be in the class - having to pass a
dummy template parameter is ugly. I think you could make a separate class with
the same layout, and static_cast to it - this will make the assumption of same
layout explicit.

> +#if PLATFORM(WIN_OS)
> +// ThreadSpecificThreadExit should be called each time when a thread is
detached.
> +extern void ThreadSpecificThreadExit();
> +#endif

Will calling this function be a JavaScriptCore API requirement? Then it should
be in API/JSBase.h, and named differently, but that sounds like a particularly
unfortunate requirement to impose on clients. Many libraries have
initialization calls, but few need explicit shutdown, especially on each
thread. I don't see a good solution, maybe we should discuss this on the
mailing list.

> +    m_key = TlsAlloc();
> +    if (m_key == TLS_OUT_OF_INDEXES)
> +	   CRASH();
> +
> +    int slot = InterlockedIncrement(&g_tls_key_count) - 1;

It seems that TLS is almost re-implemented here, in addition to using the
platform-provided solution. I hope that there is a simpler solution.

The main issue here is that this basically changes JavaScriptCore API in an
objectionable way - I don't have a proposal, but this definitely needs to be
resolved.

Sorry for not reviewing this earlier, I overlooked the patch when it was added.