[webkit-reviews] review denied: [Bug 78389] wtf/ThreadingWin.cpp doesn't build for 64-bit Windows : [Attachment 127165] Patch v4

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Kalev Lember
<kalevlember at gmail.com>'s request for review:
Bug 78389: wtf/ThreadingWin.cpp doesn't build for 64-bit Windows
https://bugs.webkit.org/show_bug.cgi?id=78389

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

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=127165&action=review


Nice! I think this is really close!

> Source/JavaScriptCore/wtf/Threading.cpp:95
> +typedef void* (*CompatFunction)(void* argument);

Maybe ThreadFunctionWithReturnValue would be a better name?

> Source/JavaScriptCore/wtf/Threading.cpp:99
> +struct CompatInvocation {

And this could be ThreadFunctionWithReturnValueInvocation.

> Source/JavaScriptCore/wtf/Threading.cpp:112
> +    OwnPtr<CompatInvocation> invocation =
adoptPtr(static_cast<CompatInvocation*>(param));

I'd add a comment here:

// Balanced by .leakPtr() in createThread.

> Source/JavaScriptCore/wtf/Threading.cpp:123
> +    OwnPtr<CompatInvocation> invocation = adoptPtr(new
CompatInvocation(entryPoint, data));
> +    ThreadIdentifier id = createThread(compatEntryPoint, invocation.get(),
name);
> +
> +    // The thread will take ownership of invocation.
> +    if (!invocation.leakPtr())
> +	   return 0;

You can pass the result of .leakPtr() directly to createThread:

// Balanced by adoptPtr() in compatEntryPoint.
return createThread(compatEntryPoint, invocation.leakPtr(), name);

> Source/JavaScriptCore/wtf/Threading.cpp:147
> +    OwnPtr<CompatInvocation> invocation = adoptPtr(new
CompatInvocation(entryPoint, data));
> +    ThreadIdentifier id = createThread(compatEntryPoint, invocation.get(),
0);
> +
> +    // The thread will take ownership of invocation.
> +    if (!invocation.leakPtr())
> +	   return 0;

Same advice here as above.

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:160
> +    OwnPtr<ThreadFunctionInvocation> invocation =
adoptPtr(static_cast<ThreadFunctionInvocation*>(param));

I'd add a comment here:

// Balanced by .leakPtr() in createThreadInternal.

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:200
> +    // The thread will take ownership of invocation.
> +    if (!invocation.leakPtr())
> +	   return 0;

Our normal phrasing is "Balanced by adoptPtr() in wtfThreadEntryPoint."

This code is essentially checking whether the allocation of
ThreadFunctionInvocation succeeded. This isn't something we typically do in
WebKit. I'm not sure how to deal with the WARN_UNUSED_RESULT though. It looks
like PingLoader.cpp does this:


// Leak the ping loader, since it will kill itself as soon as it receives a
response.
PingLoader* leakedPingLoader = pingLoader.leakPtr();
UNUSED_PARAM(leakedPingLoader);

> Source/JavaScriptCore/wtf/ThreadingPthreads.cpp:216
> +    // The thread will take ownership of invocation.
> +    if (!invocation.leakPtr())
> +	   return 0;

Same comments as above.