[PATCH] D11781: Refactored pthread usage in libcxx

[Fulvio Esposito via cfe-commits]

espositofulvio added inline comments.

================
Comment at: include/__mutex_base:19
@@ +18,3 @@
+#ifndef _WIN32
+#include <support/pthread/mutex.hpp>
+#endif
----------------
jroelofs wrote:
> I think it might make sense to create a file: `<support/mutex.h>` where the contents are just:
> 
> ```
> #ifndef _WIN32
> #include <support/pthread/mutex.hpp>
> #endif
> ```
> 
> And include that file here so that the platform decision of "which mutex implementation should we use" doesn't have these #ifndef guards spread everywhere.
> 
> Maybe you could also add some defines & guards to enforce the idea that `<support/pthread/mutex.hpp>` is never included directly, but rather only via `<support/mutex.h>`.
> 
> Same should go for thread.hpp, too.
A very good idea indeed. I'll make the change and update the patch.

================
Comment at: include/__mutex_base:246
@@ -266,3 +245,3 @@
 
-class _LIBCPP_TYPE_VIS condition_variable
+class _LIBCPP_TYPE_VIS condition_variable : private __libcxx_support::condition_variable
 {
----------------
theraven wrote:
> Does this change the ABI for a mutex on *NIX platforms?  We can't change the class layout for existing platforms (though we can indirect things via typedefs).
My hunch is that it doesn't, std::condition_variable has no data member now and the first base class (__libcxx_support::condition_variable) contains only pthread_cond_t which will be effectively laid out at the starting address of the object,  as it was previously for std::condition_variable,. I will check this out later in the evening though.

================
Comment at: include/mutex:182
@@ -181,2 +181,3 @@
 #endif
-#include <sched.h>
+#ifndef _WIN32
+#include <support/pthread/mutex.hpp>
----------------
theraven wrote:
> As above, there should probably be in a cross-platform support file that includes these.  In particular, not-win32 is not the correct condition for uses-pthreads.  We should probably have something in __config that determines the thread library to use.  It would be quite nice to have a C11 thread back end, for example (though most implementations currently wrap pthreads).
In this case having a series of #ifdef __FreeBSD__ (or _WIN32, etc.) doesn't quite cut it as we want to be able to select C11 or pthread on most of them and on Windows one day it might even be C11, pthread or the win32 api. I guess the alternative is to provide a cmake variable that default to something different on each platform?



Repository:
  rL LLVM

http://reviews.llvm.org/D11781