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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Not finished looking through this but, here are some comments.

Overall, I really like this change. It looks fantastic. It seems like a much cleaner/simpler implementation. Thanks for all the time you've spent on this.



================
Comment at: libcxx/include/__threading_support:29
 # include <sched.h>
+# include <semaphore.h>
+# ifdef __APPLE__
----------------
Are all the platforms we support guaranteed to have this header (they very well might, I just don't know)?


================
Comment at: libcxx/include/__threading_support:31
+# ifdef __APPLE__
+#  define _LIBCPP_NO_NATIVE_SEMAPHORES
+# endif
----------------
Is this macro still needed?


================
Comment at: libcxx/include/__threading_support:33
+# endif
+# ifndef _LIBCPP_NO_NATIVE_SEMAPHORES
+#  define _LIBCPP_USE_NATIVE_SEMAPHORES
----------------
This could be an else block, no?


================
Comment at: libcxx/include/__threading_support:77
+typedef sem_t __libcpp_semaphore_t;
+#define _LIBCPP_SEMAPHORE_MAX SEM_VALUE_MAX
+
----------------
Are there any platforms where `SEM_VALUE_MAX` doesn't exist? Maybe some BSD platform? Could you check that it is defined (or maybe that we're on a certain platform)?


================
Comment at: libcxx/include/atomic:2447
+    _LIBCPP_INLINE_VISIBILITY
+    bool test(memory_order __m = memory_order_seq_cst) const volatile _NOEXCEPT
+        {return _LIBCPP_ATOMIC_FLAG_TYPE(true)==__cxx_atomic_load(&__a_, __m);}
----------------
I might just be missing it but, is this in the standard? Otherwise, could you mangle it?


================
Comment at: libcxx/include/atomic:2448
+    bool test(memory_order __m = memory_order_seq_cst) const volatile _NOEXCEPT
+        {return _LIBCPP_ATOMIC_FLAG_TYPE(true)==__cxx_atomic_load(&__a_, __m);}
+    _LIBCPP_INLINE_VISIBILITY
----------------
nit: space between the equals.


================
Comment at: libcxx/include/barrier:274
+public:
+    using arrival_token = typename __barrier_base<_CompletionF>::arrival_token;
+
----------------
Will `__barrier_base` be defined if `_LIBCPP_HAS_NO_TREE_BARRIER` isn't?


================
Comment at: libcxx/include/latch:53
+#if _LIBCPP_STD_VER < 11
+# error <latch> requires C++11 or later
+#endif
----------------
Can we make it so that these headers just don't exist before C++11 via cmake? That might be a nicer way to fail. 


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480