[webkit-reviews] review denied: [Bug 22614] Need to add Win32 implementation of ThreadSpecific. : [Attachment 26105] Proposed Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Dec 23 14:53:08 PST 2008
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.
More information about the webkit-reviews
mailing list