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

[Olivier Giroux via Phabricator via libcxx-commits]

__simt__ marked 11 inline comments as done.
__simt__ added a comment.

Replies. Also noting that I will include an update to the implementation status HTML page.



================
Comment at: libcxx/include/__threading_support:34
+# ifndef _LIBCPP_NO_NATIVE_SEMAPHORES
+#  define _LIBCPP_USE_NATIVE_SEMAPHORES
+# endif
----------------
ldionne wrote:
> Any reason to introduce this macro instead of just use `!defined(_LIBCPP_NO_NATIVE_SEMAPHORES)` when you need it?
I was going to say it’s more convenient for me, but I’m not even convinced.

I can streamline this.


================
Comment at: libcxx/include/__threading_support:513
+
+#define _LIBCPP_POLLING_COUNT 64
+
----------------
ldionne wrote:
> `static constexpr`?
Yes.


================
Comment at: libcxx/include/atomic:550
 #include <__config>
+#include <__threading_support>
 #include <cstddef>
----------------
ldionne wrote:
> Just wondering: will this in any way make it harder to support `<atomic>` in freestanding?
A bit, yeah. There are a few ways to proceed.


================
Comment at: libcxx/include/atomic:1465
+#ifdef __linux__
+    #define _LIBCPP_PLATFORM_WAIT_STATE_BITS 32
+    using __cxx_contention_t = int32_t;
----------------
ldionne wrote:
> I'm not seeing this used anywhere -- am I missing something?
I intend to use that to implement the normative encouragement that atomic_signed/unsigned[...]_t should be the efficient ones for waiting. I added those types below, but they don’t follow that encouragement.


================
Comment at: libcxx/include/semaphore:61
+
+#if _LIBCPP_STD_VER < 11
+# error <semaphore> is requires C++11 or later
----------------
ldionne wrote:
> 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.
Yep


================
Comment at: libcxx/src/atomic.cpp:47
+
+#elif defined(__APPLE__) // && some condition here for Catalina+
+
----------------
ldionne wrote:
> 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)`.
Thanks!


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480