[PATCH] D19415: [libcxx][rfc] Externalized threading support

[Ben Craig via cfe-commits]

bcraig added a subscriber: bcraig.
bcraig added a comment.

Note:  My opinion doesn't count for much as far as community acceptance goes.

I'm fine with the idea.  I think there is at least one significant conformance hurdle that you will need to address though.  I also have some commentary on the implementation details.


================
Comment at: include/__mutex_base:43
@@ +42,3 @@
+#if defined(_LIBCPP_THREAD_API_EXTERNAL)
+    mutex() _NOEXCEPT {__libcpp_os_support::__os_mutex_init(&__m_);}
+#elif !defined(_LIBCPP_HAS_NO_CONSTEXPR)
----------------
This is not C++14 conformant.  The default constructor for mutex() needs to be constexpr.

I'm not sure if there is a good way around this issue given the problem you are trying to solve.  There have been similar constexpr issues with this constructor with regards to musl libc.

================
Comment at: include/__os_support:23
@@ +22,3 @@
+// Mutexes
+struct __mutex_t;
+typedef struct __mutex_t* __os_mutex;
----------------
I would prefer that the internal types use an under-under-libcpp __libcpp prefix.  I've had issues in the past with my under-under symbol names colliding with those from the underlying C library, especially for obvious names like under-under-mutex_t.

================
Comment at: src/algorithm.cpp:65
@@ +64,3 @@
+# elif defined(_LIBCPP_THREAD_API_EXTERNAL)
+    __rs_mut.lock();
+# else
----------------
I would rather the new branch be used across the board.  I don't like littering the generic implementation files with preprocessor blocks everywhere.

Stated differently, get rid of 
```
__libcpp_os_support::__os_mutex __rs_mut
```
and just use 
```
mutex __rs_mut
```
everywhere instead.

================
Comment at: src/mutex.cpp:31
@@ -30,1 +30,3 @@
+#elif defined(_LIBCPP_THREAD_API_EXTERNAL)
+    __os_mutex_destroy(__m_);
 #else
----------------
My preference is for all of this to hide behind the __os_mutex_destroy call, instead of having the preprocessor block here.  It looks like that has already been discussed in a prior review though ( http://reviews.llvm.org/D11781 ).


http://reviews.llvm.org/D19415