Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads

From: Andrzej K. Haczewski <ahaczewski@gmail.com>
Date: Nov 4, 2009, 08:17 UTC
Message-ID: <16cee31f0911040017x70c7dfeev6696c3ea8f7ffe56@mail.gmail.com>
In-Reply-To: <alpine.DEB.1.00.0911040031210.4985@pacific.mpi-cbg.de>

2009/11/4 Johannes Schindelin <Johannes.Schindelin@gmx.de>:

> Could you please add the reasoning from the cover letter to this commit
> message?  And add a sign-off?

Sure, will do so for next submission of that patch.

Show 6 quoted lines

> It is unlikely that an #ifdef "contamination" of this extent will go
> through easily, but I have a suggestion that may make your patch both
> easier to read and more likely to be accepted into git.git: Try to wrap
> the win32 calls into pthread-compatible function signatures.  Then you can
> add a compat/win32/pthread.h and not even touch core files of git.git at
> all.

First of all I didn't want to use wrappers because (if not inlined) they introduce one additional call, that can be avoided with #defines (as you can see even pthread_init can be done with macro). Second reason is that I didn't want to create wrapping structures that would need to be initialized / allocated / tracked. That patch translates pthread calls to purely Win32 calls without anything in between.

Here are my reasoning for some of these #ifdefs and what can be done and what can't (without using wrappers):

1. Thread routine has very different signature:
void *__cdecl func(void *); /* pthreads */
uint32_t __stdcall func(void *); /* Windows API */
First I thought it might be a problem to do (especially return value,
which is different size for 64-bit architectures), but since Git
doesn't use return value, it can be done.

2. Initialization of CRITICAL_SECTION and SEMAPHORE (used by condition
variables implementation). These need explicit initialization on
Windows and can't be done statically with PTHREAD_MUTEX_INITIALIZER
and PTHREAD_COND_INITIALIZER. There's no easy way around that (read:
it needs wrappers).

> Oh, and you definitely do not want to copy-paste err_win_to_posix().  You
> definitely want to reuse the existing instance.

Yeah, that was lazy, mea culpa.

I'll resubmit the patch with some fixes shortly, Andrew