[PATCH] D28220: provide Win32 native threading

[Andrey Khalyavin via Phabricator via cfe-commits]

halyavin added inline comments.


================
Comment at: include/__threading_support:458
+                               __libcpp_mutex_reference&& __m,
+                               timespec* __ts)
+{
----------------
In posix, pthread_cond_timedwait uses absolute time but we use relative time here.


================
Comment at: include/__threading_support:460
+{
+  using namespace _VSTD::chrono;
+  auto timeout = seconds(__ts->tv_sec) + nanoseconds(__ts->tv_nsec);
----------------
We need to include <chrono> to use types and functions from this namespace.


================
Comment at: include/__threading_support:464
+
+  _LIBCPP_ASSERT(timeout_ms.count() < _VSTD::numeric_limits<DWORD>::max(),
+                 "timeout duration overflows");
----------------
You have done it accidentally right. I realized there is a second problem here - if timeout equals to INFINITE, the thread will wait forever. INFINITE equals to (DWORD)-1, so the strict sign in the assert is required. But for sanity sake _VSTD::numeric_limits<DWORD>::max() should be replaced with INFINITE.  


================
Comment at: include/__threading_support:486
+static inline _LIBCPP_ALWAYS_INLINE BOOL CALLBACK
+__libcpp_init_once_trampoline(PINIT_ONCE InitOnce, PVOID Parameter,
+                              PVOID *Context)
----------------
We need underscores for parameters and init_routine.


================
Comment at: include/__threading_support:497
+
+int __libcpp_execute_once(__libcpp_exec_once_flag* flag,
+                          void (*init_routine)(void))
----------------
Underscores for parameters are missing.


================
Comment at: include/__threading_support:527
+static inline _LIBCPP_ALWAYS_INLINE unsigned int WINAPI
+__libcpp_thread_trampoline(void *__data)
+{
----------------
Trampolines will never be inlined. Should we put them in support *.cpp instead? The downside is new public symbols which can't be changed without breaking backward compatibility. The upside is that we will have only one copy of each trampoline. What do you think?


================
Comment at: include/__threading_support:531
+      *(__libcpp_thread_trampoline_data*)__data;
+  _VSTD::free(__data);
+  return reinterpret_cast<unsigned int>(data.__func(data.__arg));
----------------
Do we need #include <cstdlib> for free() and malloc() now? Can we use new/delete instead?

BTW, What is the purpose of _VSTD? I think it is used to prevent argument-dependent lookup and so is not needed for free and malloc.


================
Comment at: include/__threading_support:532
+  _VSTD::free(__data);
+  return reinterpret_cast<unsigned int>(data.__func(data.__arg));
+}
----------------
Should we even try to pass thread exit code, given that sizeof(unsigned int) < sizeof(void*) on 64-bit system? std::thread doesn't support thread exit code anyway.


Repository:
  rL LLVM

https://reviews.llvm.org/D28220