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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/atomic:1477
+    if (0 != __cxx_atomic_exchange(&__c->__waiters, (ptrdiff_t)0, memory_order_relaxed))
+        __libcpp_platform_wake((_Tp*)__a, true);
+}
----------------
I think this can be a `static_cast`.


================
Comment at: libcxx/include/atomic:1533
+        return;
+    if(0 != __cxx_atomic_exchange(&__c->__credit, (ptrdiff_t)0, memory_order_relaxed)) {
+        __libcpp_mutex_lock(&__c->__mutex);
----------------
Might be wrong but, can't this be a CAS (because you are both comparing and swapping `0`)?


================
Comment at: libcxx/include/atomic:1535
+        __libcpp_mutex_lock(&__c->__mutex);
+        __libcpp_mutex_unlock(&__c->__mutex);
+        __libcpp_condvar_broadcast(&__c->__condvar);
----------------
Why do you lock, then immediately unlock the mutex here?


================
Comment at: libcxx/include/atomic:1563
+            return;
+        if(__i < 12)
+            __libcpp_thread_yield_processor();
----------------
Is this another "magic" number? If so, can it be a macro too? 


================
Comment at: libcxx/include/atomic:1696
         {return __cxx_atomic_compare_exchange_strong(&__a_, &__e, __d, __m, __m);}
 
+    _LIBCPP_INLINE_VISIBILITY void wait(_Tp __v, memory_order __m = memory_order_seq_cst) const volatile _NOEXCEPT
----------------
This should only be defined after C++17.


================
Comment at: libcxx/include/atomic:1697
 
+    _LIBCPP_INLINE_VISIBILITY void wait(_Tp __v, memory_order __m = memory_order_seq_cst) const volatile _NOEXCEPT
+        {__cxx_atomic_wait(&__a_, __v, __m);}
----------------
You can use `memory_order::seq_cst` here.


================
Comment at: libcxx/include/atomic:1821
     typedef __atomic_base<_Tp> __base;
+    using value_type = _Tp;
     _LIBCPP_INLINE_VISIBILITY
----------------
Is this available in C++03? If so, use `typedef`. 

Actually, do we support any compilers that don't support `using` type aliases?


================
Comment at: libcxx/include/semaphore:105
+        else
+            __cxx_atomic_notify_one(&__count.__a_, &__contention);
+#endif
----------------
Does `__cxx_atomic_notify_one` always call out to `__cxx_atomic_notify_all`? If so, can we get rid of `__cxx_atomic_notify_one`?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480