[webkit-reviews] review denied: [Bug 16464] Modify WebCore to use win32 thread primitives : [Attachment 17950] Patch to add win32-based threading implementation (includes changes to build system).

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brent Fulgham <bfulgham at gmail.com>'s
request for review:
Bug 16464: Modify WebCore to use win32 thread primitives
http://bugs.webkit.org/show_bug.cgi?id=16464

Attachment 17950: Patch to add win32-based threading implementation (includes
changes to build system).
http://bugs.webkit.org/attachment.cgi?id=17950&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good.

I would have expected the Mutex.cpp file to be named MutexWin.cpp. We've been
using both filenames and directory names to distinguish these platform-specific
files.

These types with _t in their names aren't needed. It can just be:

    struct PlatformMutex { };

Rather than:

    struct PlatformMutex_t { };
    typedef PlatformMutex_t PlatformMutex.

And in fact the extra types don't do any good.

Also, we don't normally format members with their names lined up at tab stops.

 53	// Warning:  pthread_mutex_trylock will return an error
 54	// if the lock is already owned by the current thread.	Win32
 55	// threads make no such complaint.

This comment will be confusing in the future. At the moment you are writing
this, pthreads seems relevant, but later people will wonder why you mention it.
I think you need to say more clearly that this is modeled after the
pthread_mutex_trylock semantic, and why it is.

 68	    return true;
 69	} else if (result == 0)   // ... failed

We normally don't do else after return. Further, there's no need for the else
to check the opposite of the if condition. The style we normally use is "early
exit". If you straighten out the logic here there won't be an unreached code
that uses ASSERT(false). By the way, when it does arise we write that as
ASSERT_NOT_REACHED().

If you're going to quote the Boost license, it needs to be at the top of the
file with the other license, not inside the contents.

 70 static const long MAX_SEMAPHORE_COUNT = ((int) ((~0u) >> 1));

We normally do not use the ALL_CAPS form for things that are not macros. Also,
since this constant is a long, it doesn't really make sense that it uses
unsigned and int in its declaration. It should also use static_cast rather than
C-style cast and fewer extraneous parentheses and it would be easier to read.

 102	 // Enter the wait state...

I'm not a big fan of "..." in cases like this. A normal period would do as
well.

3232 #include "Logging.h"
3333 #include "Page.h"
 34 
 35 #include <wtf/HashMap.h>
3436 #include <windows.h>
 37 #include <errno.h>

We sort includes alphabetically and we don't put spaces between the quoted and
angle bracket ones.

 66 static ThreadIdentifier identifierByThreadHandle(const HANDLE threadHandle)


The const here doesn't mean anything useful. It just says that this argument
isn't modified inside the function and there's no reason for this particular
function to have it.

 72	    const HANDLE currHandle = i->second;

The const here also has limited value.

 74		return (ThreadIdentifier)i->first;

This should use static_cast. But I'm surprised a cast is needed at all. This
may be a problem under 64-bit.

 88	MutexLocker locker(threadMapMutex());
 89 
 90	ASSERT(threadMap().contains(id));
 91	
 92	threadMap().remove(id);

Would read better without the blank lines.

 101	 if (0 == threadHandle) {

We use "!threadHandle" for cases like this.

 106	 threadID = (ThreadIdentifier)threadIdentifier;

And static_cast here.

 143	 return (ThreadIdentifier)::GetCurrentThreadId ();

And here, and no space before the () in the function call.

These points of style are relatively minor, but review- so we can get them
fixed. The most important one is probably the extra code paths in
Mutex::tryLock.