On Fri, Jul 1, 2011 at 15:04, <stef...@apache.org> wrote: >... > +++ subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h Fri > Jul 1 19:04:29 2011 >... > +#ifndef SVN_MUTEX_H > +#define SVN_MUTEX_H > + > +#include <apr_thread_mutex.h> > +#include "svn_error.h"
Typical style is to put blank lines between the "natural groupings" of includes. > + > +#ifdef __cplusplus > +extern "C" { > +#endif /* __cplusplus */ > + > +/** > + * This is a simple wrapper around @c apr_thread_mutex_t and will be a > + * valid identifier even if APR does not support threading. > + */ Actually, I don't think it will be valid: if !APR_HAS_THREADS, then you have an empty structure. I think some compiles will barf on that, so you probably want "int unused;" in that situation. >... > +/** Initialize the @a *mutex with a lifetime defined by @a pool, if > + * @a enable_mutex is TRUE and with @c NULL otherwise. If @a enable_mutex > + * is set but threading is not supported by APR, this function returns an > + * @c APR_ENOTIMPL error. > + */ > +svn_error_t * > +svn_mutex__init(svn_mutex__t *mutex, > + svn_boolean_t enable_mutex, > + apr_pool_t *pool); > + > +/** Acquire the @a mutex, if that has been enabled in @ref svn_mutex__init. > + * Make sure to call @ref svn_mutex__unlock some time later in the same > + * thread to release the mutex again. Recursive locking are not supported. > + */ > +svn_error_t * > +svn_mutex__lock(svn_mutex__t mutex); In the Subversion source code, we generally do not pass structures as parameters. I'm not even sure if I can find an example. The code "always" passes a pointer-to-structure. This also has an impact on the signature for svn_mutex__init, and the unlock function, of course. > +/** Release the @a mutex, previously acquired using @ref svn_mutex__lock > + * that has been enabled in @ref svn_mutex__init. > + */ > +svn_error_t * > +svn_mutex__unlock(svn_mutex__t mutex, > + svn_error_t *err); > + > +#ifdef __cplusplus > +} > +#endif /* __cplusplus */ I agree with Daniel's suggestion to add a "with_lock" function that invokes a callback with the mutex held. That is probably the safest way to use the mutexes, and it will always guarantee they are unlocked (eg. in the face of an error). I see that you're accepting an ERR as a parameter on the unlock, presumably to try and handle these error-exit situations. My preference would be to completely omit the explicit lock/unlock functions, and *only* have the with_lock concept. I think it makes for better/safer code. >... > +++ subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c Fri Jul > 1 19:04:29 2011 >... > +#else > + if (enable_mutex) > + return svn_error_wrap_apr(APR_ENOTIMPL, _("APR doesn't support > threads")); > +#endif Euh. You can just return one of the SVN_ERR_ codes here. Wrapping an APR error doesn't make much sense :-P Maybe SVN_ERR_UNSUPPORTED_FEATURE ? >... Cheers, -g