[pulseaudio-discuss] [PATCH RFCv2 21/27] once: Inline functions

[David Henningsson]

Looks good in general, see comments below

On 2014-10-28 20:46, Peter Meerwald wrote:
> From: Peter Meerwald <p.meerwald at bct-electronic.com>

Missing a good commit message.

> Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> ---
>   src/pulsecore/once.c | 18 +-----------------
>   src/pulsecore/once.h | 27 +++++++++++++++++++++++----
>   2 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/src/pulsecore/once.c b/src/pulsecore/once.c
> index 16059c3..cac8cda 100644
> --- a/src/pulsecore/once.c
> +++ b/src/pulsecore/once.c
> @@ -30,14 +30,9 @@
>   /* See http://www.hpl.hp.com/research/linux/atomic_ops/example.php4 for the
>    * reference algorithm used here. */
>
> -bool pa_once_begin(pa_once *control) {
> +bool pa_once_doit(pa_once *control) {
>       pa_mutex *m;
>
> -    pa_assert(control);
> -
> -    if (pa_atomic_load(&control->done))
> -        return false;
> -
>       /* Caveat: We have to make sure that the once func has completed
>        * before returning, even if the once func is not actually
>        * executed by us. Hence the awkward locking. */
> @@ -64,14 +59,3 @@ void pa_once_end(pa_once *control) {
>       m = pa_static_mutex_get(&control->mutex, false, false);
>       pa_mutex_unlock(m);
>   }
> -
> -/* Not reentrant -- how could it be? */
> -void pa_run_once(pa_once *control, pa_once_func_t func) {
> -    pa_assert(control);
> -    pa_assert(func);
> -
> -    if (pa_once_begin(control)) {
> -        func();
> -        pa_once_end(control);
> -    }
> -}
> diff --git a/src/pulsecore/once.h b/src/pulsecore/once.h
> index 460a700..3d528a7 100644
> --- a/src/pulsecore/once.h
> +++ b/src/pulsecore/once.h
> @@ -26,18 +26,27 @@
>   #include <pulsecore/mutex.h>
>
>   typedef struct pa_once {
> -    pa_static_mutex mutex;
>       pa_atomic_t done;
> +    pa_static_mutex mutex;

Any particular reason these changed order?

>   } pa_once;
>
>   #define PA_ONCE_INIT                                                    \
>       {                                                                   \
> +        .done = PA_ATOMIC_INIT(0),                                      \
>           .mutex = PA_STATIC_MUTEX_INIT,                                  \
> -        .done = PA_ATOMIC_INIT(0)                                       \

Ditto

>       }
>
>   /* Not to be called directly, use the macros defined below instead */
> -bool pa_once_begin(pa_once *o);
> +bool pa_once_doit(pa_once *control);

You would typically not like to expose this function, but it is 
necessary as the beginning of the function was inlined. Should perhaps 
be called "pa_once_begin_internal" and have a comment saying that one 
should call pa_once_begin instead of this function.

> +
> +static inline bool pa_once_begin(pa_once *control) {
> +    pa_assert(control);
> +
> +    if (pa_atomic_load(&control->done))
> +        return false;
> +
> +    return pa_once_doit(control);
> +}
>   void pa_once_end(pa_once *o);
>
>   #define PA_ONCE_BEGIN                                                   \
> @@ -68,6 +77,16 @@ void pa_once_end(pa_once *o);
>
>   /* Same API but calls a function */
>   typedef void (*pa_once_func_t) (void);
> -void pa_run_once(pa_once *o, pa_once_func_t f);
> +
> +/* Not reentrant -- how could it be? */
> +static inline void pa_run_once(pa_once *control, pa_once_func_t func) {
> +    pa_assert(control);
> +    pa_assert(func);
> +
> +    if (pa_once_begin(control)) {
> +        func();
> +        pa_once_end(control);
> +    }
> +}
>
>   #endif
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic