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

[Peter Meerwald]

> 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?

ah, I was experimenting with alignment of atomic variables

but I have no good argument either way -- should keep the original 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.

will do, thanks!

> 
> > +
> > +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
> >
> 
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)