diff mbox

[v3] use NSDMI in C++11 mutex types

Message ID CAH6eHdSB44AjrtQQYtc2ohXA-_shniFG7n65_UANE354v45UrQ@mail.gmail.com
State New
Headers show

Commit Message

Jonathan Wakely Oct. 4, 2011, 9:53 p.m. UTC 
Now that G++ supports non-static data member initializers, I want to
use it for initializing the __gthread_mutex_t members of std::mutex
and friends:

__native_type  _M_mutex = __GTHREAD_MUTEX_INIT;

This is more portable than our current code, which does:

 constexpr mutex() noexcept : _M_mutex(__GTHREAD_MUTEX_INIT) { }

This is not valid if  __gthread_mutex_t is an array type and the
initializer is e.g. {x,y}

Using a NSDMI is OK, because that's exactly the syntax that
PTHREAD_MUTEX_INIT is designed for:

pthread_mutex_t m = PTHREAD_MUTEX_INIT;

(This patch doesn't include it, but std::condition_variable's use of
__GTHREAD_COND_INIT could also use a NSDMI.)

The attached patch not only does that, but also removes duplication by
creating a __mutex_base class, inherited by std::mutex and
std::timed_mutex, and a __recursive_mutex_base, inherited by
std::recursive_mutex and std::recursive_timed_mutex. The
initialization and destruction of the native mutex types is done in
the base classes.  As required, std::mutex and std::recursive_mutex
are still standard layout types with this change.

Does anyone have any comments or objections to going in this
direction?  If the new base classes aren't OK the NSDMI syntax could
still be used, just without refactoring to remove the code
duplication.

Comments

Benjamin Kosnik Oct. 6, 2011, 10:15 p.m. UTC | #1 
> Does anyone have any comments or objections to going in this
> direction?  If the new base classes aren't OK the NSDMI syntax could
> still be used, just without refactoring to remove the code
> duplication.

I like where you are going here. This looks good to me.

-benjamin
Jonathan Wakely Oct. 6, 2011, 11:43 p.m. UTC | #2 
On 6 October 2011 23:15, Benjamin Kosnik wrote:
>
>> Does anyone have any comments or objections to going in this
>> direction?  If the new base classes aren't OK the NSDMI syntax could
>> still be used, just without refactoring to remove the code
>> duplication.
>
> I like where you are going here. This looks good to me.

OK, thanks.  I think I might tackle http://gcc.gnu.org/PR50196 first,
making everything except Timed Mutex types available on Mac OS X, then
come back to this refactoring to use NSDMIs.  That way if the
refactoring needs to be backed out for any reason, we can still keep
the fix for PR 50196, which actually adds functionality rather than
just making the code slightly more portable and maintainable.
diff mbox

Patch

Index: include/std/mutex
===================================================================
--- include/std/mutex	(revision 179472)
+++ include/std/mutex	(working copy)
@@ -52,6 +52,94 @@  namespace std _GLIBCXX_VISIBILITY(defaul
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
+  // Common base class for std::mutex and std::timed_mutex
+  class __mutex_base
+  {
+  protected:
+    typedef __gthread_mutex_t			__native_type;
+
+#ifdef __GTHREAD_MUTEX_INIT
+    __native_type  _M_mutex = __GTHREAD_MUTEX_INIT;
+
+    constexpr __mutex_base() noexcept = default;
+#else
+    __native_type  _M_mutex;
+
+    __mutex_base() noexcept
+    {
+      // XXX EAGAIN, ENOMEM, EPERM, EBUSY(may), EINVAL(may)
+      __GTHREAD_MUTEX_INIT_FUNCTION(&_M_mutex);
+    }
+
+    ~__mutex_base() { __gthread_mutex_destroy(&_M_mutex); }
+#endif
+
+    __mutex_base(const __mutex_base&) = delete;
+    __mutex_base& operator=(const __mutex_base&) = delete;
+  };
+
+  // Common base class for std::recursive_mutex and std::timed_recursive_mutex
+  class __recursive_mutex_base
+  {
+  protected:
+    typedef __gthread_recursive_mutex_t		__native_type;
+
+    __recursive_mutex_base(const __recursive_mutex_base&) = delete;
+    __recursive_mutex_base& operator=(const __recursive_mutex_base&) = delete;
+
+#ifdef __GTHREAD_RECURSIVE_MUTEX_INIT
+    __native_type  _M_mutex = __GTHREAD_RECURSIVE_MUTEX_INIT;
+
+    __recursive_mutex_base() = default;
+#else
+    __native_type  _M_mutex;
+
+    __recursive_mutex_base()
+    {
+      // XXX EAGAIN, ENOMEM, EPERM, EBUSY(may), EINVAL(may)
+      __GTHREAD_RECURSIVE_MUTEX_INIT_FUNCTION(&_M_mutex);
+    }
+
+    ~__recursive_mutex_base()
+    { _S_destroy(&_M_mutex); }
+
+  private:
+    // FIXME: gthreads doesn't define __gthread_recursive_mutex_destroy
+    // so we need to obtain a __gthread_mutex_t to destroy
+
+    // matches when there's only one mutex type
+    template<typename _Rm>
+      static
+      typename enable_if<is_same<_Rm, __gthread_mutex_t>::value, void>::type
+      _S_destroy(_Rm* __mx)
+      { __gthread_mutex_destroy(__mx); }
+
+    // matches a recursive mutex with a member 'actual'
+    template<typename _Rm>
+      static typename enable_if<sizeof(&_Rm::actual), void>::type
+      _S_destroy(_Rm* __mx)
+      { __gthread_mutex_destroy(&__mx->actual); }
+
+    // matches a gthr-win32.h recursive mutex
+    template<typename _Rm>
+      static typename enable_if<sizeof(&_Rm::sema), void>::type
+      _S_destroy(_Rm* __mx)
+      {
+        __gthread_mutex_t __tmp;
+        _S_destroy_win32(&__tmp, __mx);
+      }
+
+    template<typename _Mx, typename _Rm>
+      static void
+      _S_destroy_win32(_Mx* __mx, _Rm const* __rmx)
+      {
+        __mx->counter = __rmx->counter;
+        __mx->sema = __rmx->sema;
+        __gthread_mutex_destroy(__mx);
+      }
+#endif
+  };
+
   /**
    * @defgroup mutexes Mutexes
    * @ingroup concurrency
@@ -61,25 +149,16 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
    */
 
   /// mutex
-  class mutex
+  class mutex : private __mutex_base
   {
-    typedef __gthread_mutex_t			__native_type;
-    __native_type  _M_mutex;
-
   public:
     typedef __native_type* 			native_handle_type;
 
 #ifdef __GTHREAD_MUTEX_INIT
-    constexpr mutex() noexcept : _M_mutex(__GTHREAD_MUTEX_INIT) { }
-#else
-    mutex() noexcept
-    {
-      // XXX EAGAIN, ENOMEM, EPERM, EBUSY(may), EINVAL(may)
-      __GTHREAD_MUTEX_INIT_FUNCTION(&_M_mutex);
-    }
-
-    ~mutex() { __gthread_mutex_destroy(&_M_mutex); }
+    constexpr
 #endif
+    mutex() noexcept = default;
+    ~mutex() = default;
 
     mutex(const mutex&) = delete;
     mutex& operator=(const mutex&) = delete;
@@ -113,66 +192,14 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     { return &_M_mutex; }
   };
 
-#ifndef __GTHREAD_RECURSIVE_MUTEX_INIT
-  // FIXME: gthreads doesn't define __gthread_recursive_mutex_destroy
-  // so we need to obtain a __gthread_mutex_t to destroy
-  class __destroy_recursive_mutex
-  {
-    template<typename _Mx, typename _Rm>
-      static void
-      _S_destroy_win32(_Mx* __mx, _Rm const* __rmx)
-      {
-        __mx->counter = __rmx->counter;
-        __mx->sema = __rmx->sema;
-        __gthread_mutex_destroy(__mx);
-      }
-
-  public:
-    // matches a gthr-win32.h recursive mutex
-    template<typename _Rm>
-      static typename enable_if<sizeof(&_Rm::sema), void>::type
-      _S_destroy(_Rm* __mx)
-      {
-        __gthread_mutex_t __tmp;
-        _S_destroy_win32(&__tmp, __mx);
-      }
-
-    // matches a recursive mutex with a member 'actual'
-    template<typename _Rm>
-      static typename enable_if<sizeof(&_Rm::actual), void>::type
-      _S_destroy(_Rm* __mx)
-      { __gthread_mutex_destroy(&__mx->actual); }
-
-    // matches when there's only one mutex type
-    template<typename _Rm>
-      static
-      typename enable_if<is_same<_Rm, __gthread_mutex_t>::value, void>::type
-      _S_destroy(_Rm* __mx)
-      { __gthread_mutex_destroy(__mx); }
-  };
-#endif
-
   /// recursive_mutex
-  class recursive_mutex
+  class recursive_mutex : private __recursive_mutex_base
   {
-    typedef __gthread_recursive_mutex_t		__native_type;
-    __native_type  _M_mutex;
-
   public:
     typedef __native_type* 			native_handle_type;
 
-#ifdef __GTHREAD_RECURSIVE_MUTEX_INIT
-    recursive_mutex() : _M_mutex(__GTHREAD_RECURSIVE_MUTEX_INIT) { }
-#else
-    recursive_mutex()
-    {
-      // XXX EAGAIN, ENOMEM, EPERM, EBUSY(may), EINVAL(may)
-      __GTHREAD_RECURSIVE_MUTEX_INIT_FUNCTION(&_M_mutex);
-    }
-
-    ~recursive_mutex()
-    { __destroy_recursive_mutex::_S_destroy(&_M_mutex); }
-#endif
+    recursive_mutex() = default;
+    ~recursive_mutex() = default;
 
     recursive_mutex(const recursive_mutex&) = delete;
     recursive_mutex& operator=(const recursive_mutex&) = delete;
@@ -207,31 +234,19 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   };
 
   /// timed_mutex
-  class timed_mutex
+  class timed_mutex : private __mutex_base
   {
-    typedef __gthread_mutex_t 		  	__native_type;
-
 #ifdef _GLIBCXX_USE_CLOCK_MONOTONIC
     typedef chrono::steady_clock 	  	__clock_t;
 #else
     typedef chrono::high_resolution_clock 	__clock_t;
 #endif
 
-    __native_type  _M_mutex;
-
   public:
     typedef __native_type* 		  	native_handle_type;
 
-#ifdef __GTHREAD_MUTEX_INIT
-    timed_mutex() : _M_mutex(__GTHREAD_MUTEX_INIT) { }
-#else
-    timed_mutex()
-    {
-      __GTHREAD_MUTEX_INIT_FUNCTION(&_M_mutex);
-    }
-
-    ~timed_mutex() { __gthread_mutex_destroy(&_M_mutex); }
-#endif
+    timed_mutex() = default;
+    ~timed_mutex() = default;
 
     timed_mutex(const timed_mutex&) = delete;
     timed_mutex& operator=(const timed_mutex&) = delete;
@@ -312,33 +327,19 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   };
 
   /// recursive_timed_mutex
-  class recursive_timed_mutex
+  class recursive_timed_mutex : private __recursive_mutex_base
   {
-    typedef __gthread_recursive_mutex_t		__native_type;
-
 #ifdef _GLIBCXX_USE_CLOCK_MONOTONIC
     typedef chrono::steady_clock 		__clock_t;
 #else
     typedef chrono::high_resolution_clock 	__clock_t;
 #endif
 
-    __native_type  _M_mutex;
-
   public:
     typedef __native_type* 			native_handle_type;
 
-#ifdef __GTHREAD_RECURSIVE_MUTEX_INIT
-    recursive_timed_mutex() : _M_mutex(__GTHREAD_RECURSIVE_MUTEX_INIT) { }
-#else
-    recursive_timed_mutex()
-    {
-      // XXX EAGAIN, ENOMEM, EPERM, EBUSY(may), EINVAL(may)
-      __GTHREAD_RECURSIVE_MUTEX_INIT_FUNCTION(&_M_mutex);
-    }
-
-    ~recursive_timed_mutex()
-    { __destroy_recursive_mutex::_S_destroy(&_M_mutex); }
-#endif
+    recursive_timed_mutex() = default;
+    ~recursive_timed_mutex() = default;
 
     recursive_timed_mutex(const recursive_timed_mutex&) = delete;
     recursive_timed_mutex& operator=(const recursive_timed_mutex&) = delete;