[Webkit-unassigned] [Bug 78389] wtf/ThreadingWin.cpp doesn't build for 64-bit Windows

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78389


Adam Roben (:aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #127165|review?                     |review-
               Flag|                            |




--- Comment #11 from Adam Roben (:aroben) <aroben at apple.com>  2012-02-17 08:25:15 PST ---
(From update of attachment 127165)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.