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

[Johannes Sixt <j.sixt@viscovery.net>]

Andrzej K. Haczewski schrieb:
> ---

You should sign-off your patches.

>  #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?

> +#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.

> +#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.

> +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.)

> +	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...?

> +{
> +	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.

> +
> +	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