apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cliff Woolley <jwool...@virginia.edu>
Subject Re: [PATCH] WIN32 - fix apr_thread_join (PR: 28460) - v.2
Date Fri, 27 Aug 2004 19:06:06 GMT
On Fri, 27 Aug 2004, Mladen Turk wrote:

> c) If thread_exit was never called before thread_join
>     do not set the retval but rather return APR_INCOMPLETE.

That's really what the unix impl does?  APR_INCOMPLETE was supposed to
mean that partial results were returned but just not quite everything the
caller asked for (as in the case of stat()ing a file but only getting back
some of the information the user wanted rather than all).  Surely this is
an error condition, right?  In that case, one of the APR_E* codes would be
more appropriate.  Or maybe I'm not correctly understanding the situation
you're describing...

--Cliff



PS, following are a couple of style nits, just fyi.

>   	unsigned temp;
> +    HANDLE handle;

Please be careful not to introduce tabs into the code (that applies to apr
as well as httpd by the way).  I usually just set my MSVC editor and vim
both to always expand tabs (I think MSVC calls it "insert spaces").

>   #ifndef _WIN32_WCE
> -    if (((*new)->td = (HANDLE)_beginthreadex(NULL,
> +    if ((handle = (HANDLE)_beginthreadex(NULL,
>                           attr && attr->stacksize > 0 ? attr->stacksize
: 0,
>                           (unsigned int (APR_THREAD_FUNC *)(void  *))dummy_worker,
>                           (*new), 0, &temp)) == 0) {

Usually when we wrap arguments, it goes like this:

calltosomereallylongfunctionnameblahblahblah(argument1,
                                             argument2,
                                             argument3);

In this case, I think the original was trying to compensate for the fact
that the arguments themselves were also very long, so it used the
alternate method of indenting four spaces from the = sign on the first
row.  Since the = sign has now shifted to the left, the arguments should
shift to the left by the same amount.


> +    else
> +        (*new)->td = handle;

Go ahead and put the {}'s around the else case here... we almost always
use them even if the block is only one line, just for defensive
programming purposes (aka, future-proofness).


Mime
View raw message