On 04.03.2015 13:23, Philip Martin wrote:
> Branko Čibej <brane_at_wandisco.com> writes:
>
>> error stacks look sane. But, before committing the change, I'd like
>> someone else to review the patch because it's just a wee bit tricky in
>> places what with all the #ifdefs.
> Looks OK, a few minor things:
>
>> Index: subversion/libsvn_subr/error.c
>> ===================================================================
>> --- subversion/libsvn_subr/error.c (revision 1663941)
>> +++ subversion/libsvn_subr/error.c (working copy)
>> @@ -28,6 +28,10 @@
>> #include <apr_pools.h>
>> #include <apr_strings.h>
>>
>> +#if defined(SVN_DEBUG) && APR_HAS_THREADS
>> +#include <apr_thread_proc.h>
>> +#endif
> Why not an unconditional include?
I prefer to keep it conditional so that bugs in fiddling with the
#ifdefs lower down will cause a compiler error.
>> -/* XXX TODO: Define mutex here #if APR_HAS_THREADS */
>> +/* No-op destructor for apr_threadkey_private_create(). */
>> +static void null_threadkey_dtor(void *stuff) {}
>> +
>> +/* Init-once function for the thread-local keys.
>> + This function will never return an error. */
> It is easier for the reader if "svn_atomic__init_once" is explict rather
> than implied by "Init-once". If svn_atomic__init_once used a typedef
> for the callback that would be even better, but that is a separate
> change.
I'll fix the comment ... but a typedef for the svn_atomic__init_once
handler wouldn't really help, since you can't use that to define a function.
-- Brane
Received on 2015-03-04 13:44:58 CET