[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1142026 - in /subversion/branches/svn_mutex/subversion: include/private/svn_mutex.h libsvn_subr/svn_mutex.c

From: Greg Stein <gstein_at_gmail.com>
Date: Fri, 1 Jul 2011 22:24:44 -0400

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

This is an archived mail posted to the Subversion Dev mailing list.