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

[Olivier Giroux via Phabricator via libcxx-commits]

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

Thanks Zoe!

Some responses ->



================
Comment at: libcxx/include/__threading_support:29
 # include <sched.h>
+# include <semaphore.h>
+# ifdef __APPLE__
----------------
zoecarver wrote:
> Are all the platforms we support guaranteed to have this header (they very well might, I just don't know)?
This header is part of Pthreads and this #include is in the Pthreads section of the header, so I think the answer is yes. The Apple case is special in that they have deprecated this header.


================
Comment at: libcxx/include/__threading_support:31
+# ifdef __APPLE__
+#  define _LIBCPP_NO_NATIVE_SEMAPHORES
+# endif
----------------
zoecarver wrote:
> Is this macro still needed?
Yes. We need a way to disable the platform native semaphores. Both Apple (in the current design) and CUDA need it.


================
Comment at: libcxx/include/__threading_support:33
+# endif
+# ifndef _LIBCPP_NO_NATIVE_SEMAPHORES
+#  define _LIBCPP_USE_NATIVE_SEMAPHORES
----------------
zoecarver wrote:
> This could be an else block, no?
It would have to be an #elif !defined(...) so that CUDA could trigger it too.


================
Comment at: libcxx/include/__threading_support:77
+typedef sem_t __libcpp_semaphore_t;
+#define _LIBCPP_SEMAPHORE_MAX SEM_VALUE_MAX
+
----------------
zoecarver wrote:
> 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)?
Like the header, it's mandated by Pthreads. It's conceivable that some BSD is non-conforming and we still want to work on it, but I don't have a BSD system handy to try.


================
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);}
----------------
zoecarver wrote:
> I might just be missing it but, is this in the standard? Otherwise, could you mangle it?
It is, in C++20.


================
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
----------------
zoecarver wrote:
> nit: space between the equals.
OK.


================
Comment at: libcxx/include/barrier:274
+public:
+    using arrival_token = typename __barrier_base<_CompletionF>::arrival_token;
+
----------------
zoecarver wrote:
> Will `__barrier_base` be defined if `_LIBCPP_HAS_NO_TREE_BARRIER` isn't?
Yes. The macro ends up selecting between two different base classes -- one is the scalable tree barrier, the other is the simpler central barrier.


================
Comment at: libcxx/include/latch:53
+#if _LIBCPP_STD_VER < 11
+# error <latch> requires C++11 or later
+#endif
----------------
zoecarver wrote:
> 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. 
I don't have this skill. :^/

But I do think that we want to support them in C++11/14/17. This author's production team is going to present it to users in these dialects, for instance.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480