Re: [PATCH 1/1] MSVC: port pthread code to native Windows threads
- From
Johannes Sixt <j.sixt@viscovery.net>
- Date
- Nov 4, 2009, 08:15 UTC
- Message-ID
- <4AF13819.7050306@viscovery.net>
- In-Reply-To
- <1257283802-29726-2-git-send-email-ahaczewski@gmail.com>
Andrzej K. Haczewski schrieb:
> ---
You should sign-off your patches.
Show 10 quoted lines
> #ifdef THREADED_DELTA_SEARCH > -#include "thread-utils.h" > -#include <pthread.h> > +# include "thread-utils.h" > +# ifndef _WIN32 > +# include <pthread.h> > +# else > +# include <winthread.h> > +# endif > #endif
Can't you just use the pthread package that is included in msysgit?
Show 12 quoted lines
> +#ifndef _WIN32 > static void *threaded_find_deltas(void *arg) > +#else > +static DWORD WINAPI threaded_find_deltas(LPVOID arg) > +#endif > ... > +#ifndef _WIN32 > return NULL; > +#else > + return 0; > +#endif > etc ...
You have far too many #ifdef in the generic code. There must be a better way to hide the implementation details of this emulation.
Show 10 quoted lines
> +#ifdef _WIN32 > + /* > + * Windows require initialization of mutex (CRITICAL_SECTION) > + * and conditional variable. > + */ > + pthread_mutex_init(&read_mutex); > + pthread_mutex_init(&cache_mutex); > + pthread_mutex_init(&progress_mutex); > + win32_cond_init(&progress_cond); > +#endif
*If* we are going to use this minimal pthreads implementation, then I think it will be OK to call pthread_*_init even on non-Windows.
Show 7 quoted lines
> +static __inline int win32_cond_init(win32_cond_t *cond)
> +{
> + cond->waiters = 0;
> +
> + InitializeCriticalSection(&cond->waiters_lock);
> +
> + cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);Wouldn't an Event object be lighter-weight? (I'm only guessing.)
Show 16 quoted lines
> + if (NULL == cond->sema)
> + return -1;
> + return 0;
> +}
> +
> +static __inline int win32_cond_destroy(win32_cond_t *cond)
> +{
> + CloseHandle(cond->sema);
> + cond->sema = NULL;
> +
> + DeleteCriticalSection(&cond->waiters_lock);
> +
> + return 0;
> +}
> +
> +static __inline int win32_cond_wait(win32_cond_t *cond, CRITICAL_SECTION *mutex)And the reason that this is not pthread_cond_wait, is...?
Show 12 quoted lines
> +{
> + DWORD result;
> + int ret = 0;
> +
> + /* we're waiting... */
> + EnterCriticalSection(&cond->waiters_lock);
> + ++cond->waiters;
> + LeaveCriticalSection(&cond->waiters_lock);
> +
> + /* unlock external mutex and wait for signal */
> + LeaveCriticalSection(mutex);
> + result = WaitForSingleObject(cond->sema, INFINITE);Releasing the mutex and entering the wait state as well as leaving the wait state and reacquiring the mutex should be atomic. Neither are in this implementation. You are not mentioning why you are implementing things like this and why this would be acceptable.
Show 11 quoted lines
> + > + if (0 != result) > + ret = -1; > + > + /* one waiter less */ > + EnterCriticalSection(&cond->waiters_lock); > + --cond->waiters; > + LeaveCriticalSection(&cond->waiters_lock); > + > + /* lock external mutex again */ > + EnterCriticalSection(mutex);
> +/* almost copy-paste code of mingw.c */
> +static int err_win_to_posix()
> +{There must be a better way than to just copy & paste this huge piece of code.
-- Hannes