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