On Fri, Jul 1, 2011 at 15:04, <stefan2_at_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
Received on 2011-07-02 04:25:21 CEST