2009/11/4 Johannes Sixt <j.sixt@xxxxxxxxxxxxx>:
> Please do not cull Cc list when you resend a patch, if possible.
Ok, will do. I was sending patch using git send-email and I just
forgot to copy Cc there. Still trying to BTW is there a way to
"reformat-patch" with new amended commit and then "resend-email"?
> After staring some time on the code, I have convinced myself that the
> pthread_cond_wait and pthread_cond_signal implementation will work *in our
> usage scenario* that has these preconditions:
But it is not impossible with that implementation. I based this
implementation on ACE (Adaptive Communication Environment, large C++
library) implementation of the same concepts. All I removed from their
implementation is cond_broadcast, since it's not used by Git. I'm sure
that ACE does the best job when it comes to threading primitives.
On resubmit I'll give more credit to ACE.
> - pthread_cond_signal is called while the mutex is held.
AFAIK that is a requirement for condition variable to be signaled
while holding the same mutex that other threads cond_wait on. I just
don't check that it is true, because Git is locking mutex.
> - We retest the condition after pthread_cond_wait returns.
>
> These conditions should be attached in BIG BOLD letters to this
> implementation; particularly, the last one.
That's also a known requirement for working with cond vars. Here's
excerpt from pthread_cond_wait man page:
When using condition variables there is always a boolean predicate
involving shared variables associated with each condition wait that is
true if the thread should proceed. Spurious wakeups from the
pthread_cond_wait() or pthread_cond_timedwait() functions may occur.
Since the return from pthread_cond_wait() or pthread_cond_timedwait()
does not imply anything about the value of this predicate, the
predicate should be re-evaluated upon such return.
> The subject is a bit misleading, IMHO. You are not porting the
> (p)threading code, but you are adding pthread_* function wrappers for Windows.
>
> Your patch adds whitespace-at-eol. Please use git show --check to see where.
>
> Please drop words from the commit message that do not make sense once this
> commit is in git's history. Look at existing commit messages to get a
> feeling for the style. Do write about "why" (motivation), "how" (design
> choices) and "how not" (dead ends that you tried).
>
Ok, thanks for pointing that out.
> I think it would be OK to drop '= PTHREAD_{MUTEX,COND}_INITIALIZER' and
> use *_init function calls without the #ifdef. Likewise for *_destroy.
Actually it won't save us many #ifdefs. There's one #ifdef for
initialization that could be saved, but then comes #ifdef for cleanup:
#if defined(THREADED_DELTA_SEARCH) && defined(_WIN32)
What you propose will remove one #ifdef _WIN32 for initialization, but
the cleanup will look almost the same:
#ifdef THREADED_DELTA_SEARCH
>
> -- Hannes
>
Thanks for awesome review, I'll fix all those returns and whitespaces
and resubmit.
--
Andrzej
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html