compnerd added inline comments.

================
Comment at: include/__threading_support:300-305
+int __libcpp_recursive_mutex_init(__libcpp_mutex_t *__m)
+{
+  InitializeSRWLock(__m);
+  return 0;
+}
+
----------------
majnemer wrote:
> I don't think you can use slim rw locks for recursive locks. I think we will 
> need to use `CRITICAL_SECTION` for those. std::recursive_mutex can't be used 
> with std::condition_variable AFAIK so all you need (I think) is recursive 
> versions of `__libcpp_mutex_...`
> 
> Recursive locks should be used far less frequently which makes it valuable, 
> IMO, to use slim rw locks for the non-recursive mutex implementation.
You are absolutely right.  That was something that I looked at originally and 
went with the CS.  However, the overhead of a tagged struct is 5 or 9 bytes 
(sizeof(void *) + 1) bytes (ignoring padding for MS ABI).  Going with that 
should give the benefits of always being able to properly initialize the CS 
instead of the kludge.


================
Comment at: include/__threading_support:355
+  // TODO(compnerd) handle spurious timeout
+  if (!SleepConditionVariableSRW(__cv, __m,
+                                 duration_cast<milliseconds>(timeout).count(),
----------------
majnemer wrote:
> I don't think it should be `__libcpp_condvar_timedwait'`s problem. 
> `__libcpp_condvar_timedwait` wraps `pthread_cond_timedwait` on POSIX 
> platforms and the caller of `__libcpp_condvar_wait` properly handles spurious 
> wakeups. The caller of `__libcpp_condvar_timedwait` probably should be 
> audited.
SG; seems that there is a single user in condition_variable.cpp


Repository:
  rL LLVM

https://reviews.llvm.org/D28220



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to