[libcxx-commits] [PATCH] D97539: [libcxx] Implement semaphores for windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/support/win32/thread_win32.cpp:11-12
 #include <__threading_support>
+#define NOMINMAX
 #include <windows.h>
 #include <process.h>
----------------
curdeius wrote:
> A bit unrelated to this patch...
> 
> I know that compile times are not a problem in libc++, but could you define `WIN32_LEAN_AND_MEAN` before including `windows.h`?
> 
> I don't think we use any of the following (https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers#faster-builds-with-smaller-header-files):
> ```
> Define WIN32_LEAN_AND_MEAN to exclude APIs such as Cryptography, DDE, RPC, Shell, and Windows Sockets.
> ```
I guess I could add that here as well.


================
Comment at: libcxx/src/support/win32/thread_win32.cpp:309
+  milliseconds __ms =
+      duration_cast<milliseconds>(__ns + chrono::nanoseconds(999999));
+  return WaitForSingleObjectEx(*(PHANDLE)__sem, __ms.count(), false) ==
----------------
curdeius wrote:
> No need for `chrono::`.
> Maybe it would be clearer to write `+ milliseconds(1) - nanoseconds(1)`?
> Or even:
> ```
> milliseconds __ms = std::chrono::ceil<milliseconds>(__ns);
> ```
> 
> Even if `ceil` is a C++17 addition, we only compile libc++ with C++17 or C++20.
That looks nice to me. FWIW this bit is a copy from line 250 (but I wouldn't go and change that in this patch).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97539/new/

https://reviews.llvm.org/D97539