[webkit-reviews] review granted: [Bug 130320] [Mac] WTFThreadData should use _pthread_getspecific_direct(). : [Attachment 226890] Patch idea

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 130320: [Mac] WTFThreadData should use _pthread_getspecific_direct().
https://bugs.webkit.org/show_bug.cgi?id=130320

Attachment 226890: Patch idea
https://bugs.webkit.org/attachment.cgi?id=226890&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226890&action=review


Looks good.

> Source/WTF/wtf/WTFThreadData.cpp:77
> +#if USE(PTHREAD_GETSPECIFIC_DIRECT)
> +static void destroyWTFThreadData(void* data)

Should leave another blank line here to match the one we left below before
#endif.

Also could consider using a lambda instead of declaring a separate function.
Not sure what is preferred style.

> Source/WTF/wtf/WTFThreadData.h:47
> +#ifdef __has_include
> +#if __has_include(<System/pthread_machdep.h>)
> +
> +#include <System/pthread_machdep.h>
> +
> +#if defined(__PTK_FRAMEWORK_JAVASCRIPTCORE_KEY0)
> +#define WTF_USE_PTHREAD_GETSPECIFIC_DIRECT 1
> +#endif
> +
> +#endif
> +#endif

I don’t like nested #if, so I’d write it like this:

#if defined(__has_include) && __has_include(<System/pthread_machdep.h>)
#include <System/pthread_machdep.h>
#endif

#if defined(__PTK_FRAMEWORK_JAVASCRIPTCORE_KEY1)
#define WTF_USE_PTHREAD_GETSPECIFIC_DIRECT 1
#endif

I think we should make the same improvement in FastMalloc.cpp; just easier to
read without the nesting. But also, I am not sure we should use the same
USE(PTHREAD_GETSPECIFIC_DIRECT) both here and there; is there a risk of
conflict if we compile all-in-one or something?

The reason I suggest checking for __PTK_FRAMEWORK_JAVASCRIPTCORE_KEY1 instead
of __PTK_FRAMEWORK_JAVASCRIPTCORE_KEY0 is that is the key we are actually using
here. Which I suppose doesn’t make sense if this is a single USE for both
places.

> Source/WTF/wtf/WTFThreadData.h:159
> +#if USE(PTHREAD_GETSPECIFIC_DIRECT)

Might consider putting the portable side of the #if first rather than the
platform-specific one.

> Source/WTF/wtf/WTFThreadData.h:177
>      // WRT WebCore:

I think this comment applies to both sides of the #if, so it should be outside
the #if.