Looks good in general, see comments below

On 2014-10-28 20:46, Peter Meerwald wrote:
From: Peter Meerwald <[email protected]>

Missing a good commit message.

Signed-off-by: Peter Meerwald <[email protected]>
---
  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
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Reply via email to