[PATCH] D19412: [libcxx] Refactor pthread usage - II

[Eric Fiselier via cfe-commits]

EricWF added a comment.

OK. I *think* this is my last round of review comments except for one specific issue. I'm still looking into the changes to the static mutex's and condition_variables in `memory.cpp` and `algorithm.cpp`. In these cases I don't want to go from compile-time initialization to run-time initialization.  This could introduce the static initialization order fiasco.


================
Comment at: include/__mutex_base:37
@@ -38,2 +36,3 @@
 {
-    pthread_mutex_t __m_;
+    typedef __libcpp_mutex_t __mutex_type;
+    __mutex_type __m_;
----------------
Why not use `__libcpp_mutex_t` directly? Sorry for yet another renaming comment.

================
Comment at: include/__mutex_base:43
@@ -43,3 +42,3 @@
 #ifndef _LIBCPP_HAS_NO_CONSTEXPR
-     constexpr mutex() _NOEXCEPT : __m_(PTHREAD_MUTEX_INITIALIZER) {}
+    constexpr mutex() _NOEXCEPT : __m_(__LIBCPP_MUTEX_INITIALIZER) {}
 #else
----------------
Only one underscore in `_LIBCPP_MUTEX_INITIALIZER`

================
Comment at: include/__threading_support:11
@@ +10,3 @@
+
+#ifndef _LIBCPP_OS_SUPPORT
+#define _LIBCPP_OS_SUPPORT
----------------
`_LIBCPP_THREADING_SUPPORT`

================
Comment at: include/__threading_support:13
@@ +12,3 @@
+#define _LIBCPP_OS_SUPPORT
+
+#ifndef _LIBCPP_HAS_NO_THREADS
----------------
```
#include <__config>

#ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
#pragma GCC system_header
#endif
```

================
Comment at: include/__threading_support:30
@@ +29,3 @@
+inline _LIBCPP_ALWAYS_INLINE
+int __libcpp_mutex_init(__libcpp_mutex_t* __m, bool is_recursive = false)
+{
----------------
Two comments:
1. This is only used to initialize a recursive mutex. Let's simply name this `__libcpp_recursive_mutex_init` and get rid of the bool parameter. 
2. This is only used in `recursive_mutex.cpp`. It's OK to keep this in `__threading_support` for now but I'll probably want to move this out of the headers later.

================
Comment at: include/__threading_support:33
@@ +32,3 @@
+    pthread_mutexattr_t attr;
+    int ec = pthread_mutexattr_init(&attr);
+    if (!ec && is_recursive) {
----------------
Nit:

Let's invert the control flow here:
```
if (ec) return ec;
ec = pthread_mutexattr_settype(...);
```

Then at the end we can just `return 0;`


================
Comment at: include/__threading_support:82
@@ +81,3 @@
+// Condition variable
+#define __LIBCPP_COND_INITIALIZER PTHREAD_COND_INITIALIZER
+typedef pthread_cond_t __libcpp_condvar_t;
----------------
Nit: `_LIBCPP_CONDVAR_INITIALIZER`

================
Comment at: include/__threading_support:119
@@ +118,3 @@
+inline _LIBCPP_ALWAYS_INLINE
+int __libcpp_thread_id_compare(__libcpp_thread_id t1, __libcpp_thread_id t2)
+{
----------------
Can we split this into `__libcpp_thread_id_equal` and `__libcpp_thread_id_less`?



================
Comment at: include/__threading_support:129
@@ +128,3 @@
+inline _LIBCPP_ALWAYS_INLINE
+int __libcpp_thread_create(__libcpp_thread_t* __t, _Func&& __f, _Arg&& __arg)
+{
----------------
This signature needs to work in C++03. Let's make the signature `__libcpp_thread_create(__libcpp_thread_t* __t, _Func* __f, _Arg* __arg)` because both `_Func` and `_Arg` should be pointers so there is no need to perfect forward them.

================
Comment at: include/__threading_support:169
@@ +168,3 @@
+inline _LIBCPP_ALWAYS_INLINE
+int __libcpp_tl_create(__libcpp_tl_key* __key, _Func&& __at_exit)
+{
----------------
This signature needs to work in C++03. Make the signature `__libcpp_tl_create(__libcpp_tl_key*, _Func*)` since the function must be a pointer.

================
Comment at: include/__threading_support:194
@@ +193,2 @@
+
+#endif // _LIBCPP_OS_SUPPORT
----------------
`_LIBCPP_THREADING_SUPPORT`

================
Comment at: src/mutex.cpp:228
@@ -249,3 +227,3 @@
 #else // !_LIBCPP_HAS_NO_THREADS
-    pthread_mutex_lock(&mut);
+    unique_lock<mutex> lk(mut);
     while (flag == 1)
----------------
This change seems incorrect. There are some paths below where we manually unlock `mut` before a `throw` or `return`. Using `unique_lock` will result in a double unlock.

I think the fix is to change all `mut.lock()` and `mut.unlock()` calls into `lk.lock()`/`lk.unlock()` calls.


http://reviews.llvm.org/D19412