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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius 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>
----------------
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.
```


================
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) ==
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97539