[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 #26484|review?                     |review-
               Flag|                            |




------- Comment #19 from ap at webkit.org  2009-01-07 00:11 PDT -------
(From update of attachment 26484)
> -#if USE(PTHREADS) || PLATFORM(WIN)
> -// Windows currently doesn't use pthreads for basic threading, but implementing destructor functions is easier
> -// with pthreads, so we use it here.
> -#include <pthread.h>
> +#if !USE(PTHREADS) && PLATFORM(WIN_OS)
> +#include <windows.h>
>  #endif

Why did you remove the include of pthread.h? I only requested removing the
comment (Windows no longer uses pthreads for ThreadSpecific, so explaining why
we do that makes no sense), not the whole section.

> +extern void ThreadSpecificThreadExit();

There is no need for "extern" here - function declarations are always extern.

+        ThreadSpecific<int>::Data* data =
static_cast<ThreadSpecific<int>::Data*>(TlsGetValue(g_tls_keys[i]));

As mentioned earlier, this needs a comment explaining why it is OK to always
use ThreadSpecific<int>.

+        if (data) {
+            data->destructor(data);
+        }

Please omit braces around this single-line statement.

> I think we do not need to add this comment because pthread_setspecific is
> called in destroy() destructor to set the TLS value to NULL and thus repeated
> call of destructor is not likely.

Actually, this is not sufficient, because a different thread specific value can
become non-NULL: we only reset the one being destroyed at the moment.

+#if !USE(PTHREADS)
+    void (*destructor)(void*);
+#endif

Oh, that's a great fix, I missed this problem in my review!

>> > +    friend void ThreadSpecificThreadExit();
>> 
>> Why is this function a friend of the class?
>
> Removed.
<...>
> Changed declaration of Data from private to public in order to remove the
> duplicate definition.

Perhaps I do not understand what problem you are solving here, but why not keep
Data private, while keeping the friend declaration? Would the duplicate you
remove be needed in that case?

This is getting close, but I think it could use one more round of review - at
least because of the removed pthread.h include, which would break the build on
some platforms!


-- 
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.