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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Thank you for all the work you've put into this patch. Here are a few more comments. Still working through all this code :)

I'm becoming increasingly worried about the amount of inline configuration and platform-specific code. I want to make sure we have tests for all of these (and ideally CI that can run all the tests).



================
Comment at: libcxx/src/atomic.cpp:28
+
+struct alignas(64) __libcpp_contention_t
+{
----------------
For what platforms is `_LIBCPP_HAS_PLATFORM_WAIT_STATE` false, and have you tested on those platforms? I'm worried that there might be compiler errors. 


================
Comment at: libcxx/src/atomic.cpp:32
+    static constexpr __libcpp_platform_contention_t __count_bit = 2;
+    __cxx_atomic_impl<__libcpp_platform_contention_t> __s = ATOMIC_VAR_INIT(0);
+};
----------------
What's the point of this macro, `ATOMIC_VAR_INIT` (I realize you didn't add it, but I'm still curious)?


================
Comment at: libcxx/src/atomic.cpp:46
+
+static constexpr size_t __libcpp_contention_state_size = (1 << 8);  /* < there's no magic in this number */
+__libcpp_contention_t __libcpp_contention_state_[ __libcpp_contention_state_size ];
----------------
This is different if `_LIBCPP_HAS_PLATFORM_WAIT_STATE` is false, right?


================
Comment at: libcxx/src/atomic.cpp:129
+    auto const __wait_bit = __libcpp_contention_t::__wait_bit;
+    if(__wait_bit & __cxx_atomic_fetch_and(__s, ~__wait_bit, memory_order_relaxed))
+        __libcpp_platform_wake(__s, __all);
----------------
Will this ever be true? 


================
Comment at: libcxx/src/atomic.cpp:133
+_LIBCPP_FUNC_VIS
+void __libcpp_platform_contention_monitor_for_wait(void const volatile* __location, 
+                                                   void* __contention_state,
----------------
same as below


================
Comment at: libcxx/src/atomic.cpp:139
+    auto* const __ret = (__libcpp_platform_contention_t*)__old_value;
+#ifndef _LIBCPP_HAS_NO_PLATFORM_WAIT_TABLE
+    if(nullptr == __s)
----------------
Is there a test for this case? What will be the effect of `__s` not getting updated? 


================
Comment at: libcxx/src/atomic.cpp:148
+_LIBCPP_FUNC_VIS
+void __libcpp_platform_contention_wait_and_cleanup(void const volatile* __location, 
+                                                   bool __cond,
----------------
Instead of having void pointers that are casted, I don't see any reason these couldn't be defined as their actual types (`__cxx_atomic_impl*` and `__libcpp_platform_contention_t*`).


================
Comment at: libcxx/src/atomic.cpp:162
+    static constexpr timespec __timeout = { 2, 0 };
+    __libcpp_platform_wait(__s, *__old, sizeof(__libcpp_platform_contention_t) > 4 ? nullptr : &__timeout);
+}
----------------
Is `__libcpp_platform_wait` defined on non-linux machines?


================
Comment at: libcxx/src/barrier.cpp:44
+#ifndef _LIBCPP_HAS_NO_THREAD_FAVORITE_BARRIER_INDEX
+            if(0 == __round) {
+                if(__current >= __current_expected)
----------------
Will this ever not //only// happen on the first iteration (if so, move it out of the loop maybe)?


================
Comment at: libcxx/src/semaphore.cpp:27
+__sem_semaphore_basic_base::~__sem_semaphore_basic_base() {
+#ifdef __APPLE__
+    auto __b = __balance.load(memory_order_relaxed);
----------------
Why is this only needed for apple?


================
Comment at: libcxx/src/semaphore.cpp:123
+        return 0 != (__old >> 32);
+    }, chrono::microseconds(50));
+    while(1) {
----------------
Where does `50` come from? Maybe make this a macro. 


================
Comment at: libcxx/src/semaphore.cpp:152
+    uint64_t __old = 0;
+    while(0 == (__old & ~0ul))
+        if(__frontbuffer.compare_exchange_weak(__old, __old + (uint64_t(__update) << 32), memory_order_acq_rel))
----------------
Is this the same as `while (__old == 0)`?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480