[libcxx-commits] [PATCH] D68480: Implementation of C++20's P1135R6 for libcxx

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

This is looking pretty good, only a few nitpicks. Once the nitpicks are addressed we can commit this and then go incrementally.



================
Comment at: libcxx/include/__threading_support:34
+# ifndef _LIBCPP_NO_NATIVE_SEMAPHORES
+#  define _LIBCPP_USE_NATIVE_SEMAPHORES
+# endif
----------------
Any reason to introduce this macro instead of just use `!defined(_LIBCPP_NO_NATIVE_SEMAPHORES)` when you need it?


================
Comment at: libcxx/include/__threading_support:513
+
+#define _LIBCPP_POLLING_COUNT 64
+
----------------
`static constexpr`?


================
Comment at: libcxx/include/atomic:550
 #include <__config>
+#include <__threading_support>
 #include <cstddef>
----------------
Just wondering: will this in any way make it harder to support `<atomic>` in freestanding?


================
Comment at: libcxx/include/atomic:1465
+#ifdef __linux__
+    #define _LIBCPP_PLATFORM_WAIT_STATE_BITS 32
+    using __cxx_contention_t = int32_t;
----------------
I'm not seeing this used anywhere -- am I missing something?


================
Comment at: libcxx/include/semaphore:61
+
+#if _LIBCPP_STD_VER < 11
+# error <semaphore> is requires C++11 or later
----------------
Olivier and I spoke offline, and it's reasonable to request C++14 at least here. Please do this in all the headers you're adding.

The idea is to avoid having headers that are supported in older dialects than they need to be (within reason), which is often a source of technical debt.


================
Comment at: libcxx/src/atomic.cpp:47
+
+#elif defined(__APPLE__) // && some condition here for Catalina+
+
----------------
In `apple_availability.h`, add:

```
#if defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)
#if __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 101500
#define _LIBCPP_USE_ULOCK
#endif
#elif defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__)
#if __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ >= 130000
#define _LIBCPP_USE_ULOCK
#endif
#elif defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__)
#if __ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__ >= 130000
#define _LIBCPP_USE_ULOCK
#endif
#elif defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__)
#if __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ >= 60000
#define _LIBCPP_USE_ULOCK
#endif
#endif // __ENVIRONMENT_.*_VERSION_MIN_REQUIRED__
```

That should do it for all platforms aligned to Mac OS 10.15. Feel free to use whatever name for `_LIBCPP_USE_ULOCK` -- `_LIBCPP_USE_APPLE_ULOCK` probably makes the most sense since it's an Apple-specific API.

Then, your `#elif` becomes `#elif defined(__APPLE__) && defined(_LIBCPP_USE_APPLE_ULOCK)`.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480