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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
