[PATCH] D28220: provide Win32 native threading

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

Added inline comments.

The main issue is that this patch needs to use the same `<__threading_support>` forward declarations as every other supported threading API; Only new definitions should be provided.



================
Comment at: include/__threading_support:33
 #include <__external_threading>
+#elif defined(_WIN32) && defined(_LIBCPP_HAS_THREAD_API_WIN32)
+#define WIN32_LEAN_AND_MEAN
----------------
Isn't checking `_LIBCPP_HAS_THREAD_API_WIN32` enough?


================
Comment at: include/__threading_support:46
+inline _LIBCPP_INLINE_VISIBILITY
+int __libcpp_recursive_mutex_init(__libcpp_mutex_t *__m);
+
----------------
The forward declarations of the `__libcpp_` threading wrapper should be shared between all API's. Please don't add your own forward declarations for Windows. 


================
Comment at: src/algorithm.cpp:51
 #ifndef _LIBCPP_HAS_NO_THREADS
-_LIBCPP_SAFE_STATIC static __libcpp_mutex_t __rs_mut = _LIBCPP_MUTEX_INITIALIZER;
+#if !defined(_WIN32)
+_LIBCPP_SAFE_STATIC
----------------
This makes me sad because not having constant initialization manifests itself as a static initialization order fiasco in libc++. If we do go this route please file a libc++ bug stating that Win32 mutex's do not provide constant initialization.

I think this should check `_LIBCPP_HAS_THREAD_API_WIN32`?


================
Comment at: src/memory.cpp:158
 _LIBCPP_SAFE_STATIC static const std::size_t __sp_mut_count = 16;
-_LIBCPP_SAFE_STATIC static __libcpp_mutex_t mut_back[__sp_mut_count] =
+#if !defined(_WIN32)
+_LIBCPP_SAFE_STATIC
----------------
I think this should check _LIBCPP_HAS_THREAD_API_WIN32?


================
Comment at: src/mutex.cpp:198
 #ifndef _LIBCPP_HAS_NO_THREADS
-_LIBCPP_SAFE_STATIC static __libcpp_mutex_t mut = _LIBCPP_MUTEX_INITIALIZER;
+#if !defined(_WIN32)
+_LIBCPP_SAFE_STATIC
----------------
I think this should check _LIBCPP_HAS_THREAD_API_WIN32?


Repository:
  rL LLVM

https://reviews.llvm.org/D28220