[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





------- Comment #21 from jianli at chromium.org  2009-01-07 13:00 PDT -------
All fixed. Thanks.

(In reply to comment #19)
> (From update of attachment 26484 [review])
> > -#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.
> 
Sorry, I accidentally removed this. I put it back.
> > +extern void ThreadSpecificThreadExit();
> 
> There is no need for "extern" here - function declarations are always extern.
> 
Removed.
> +        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>.
> 
Added comment.
> +        if (data) {
> +            data->destructor(data);
> +        }
> 
> Please omit braces around this single-line statement.
> 
Removed.
> > 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.
> 
Added the comment at the beginning of the file.
> +#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?
> 
Originally I added friend declaration in order to let ThreadSpecificThreadExit
to access some private data member in ThreadSpecific<>. It is not needed now.
However, just as you suggested, this friend declaration can be used to keep
Data private. So I put it back.
> 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.