[webkit-reviews] review denied: [Bug 22614] Need to add Win32 implementation of ThreadSpecific. : [Attachment 26484] 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 26484: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=26484&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> -#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!