[Webkit-unassigned] [Bug 22614] Need to add Win32 implementation of ThreadSpecific.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22614


ap at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26105|review?                     |review-
               Flag|                            |




------- Comment #8 from ap at webkit.org  2008-12-23 14:53 PDT -------
(From update of attachment 26105)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.