[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: include/atomic:637
+bool __cxx_atomic_compare_equal(_Tp const& __lhs, _Tp const& __rhs) {
+    return memcmp(&__lhs, &__rhs, sizeof(_Tp)) == 0;
+}
----------------
__simt__ wrote:
> EricWF wrote:
> > This doesn't seem like an implementation of an atomic comparison. 
> Right, but I needed a comparison equivalent to CAS for some user-defined types. Admittedly I'm not 100% sure that the user-defined types in the atomic tests aren't the element to blame here, maybe for not having trivial comparison that works.
I think there's an atomic builtin that would work for this. 


================
Comment at: include/atomic:1504
+    auto const __version = __cxx_atomic_load(__cxx_atomic_rebind(&__c->__version), memory_order_relaxed);
+    if (__builtin_expect(!__cxx_atomic_compare_equal(__cxx_atomic_load(__a, __order), __val), 1))
+        return;
----------------
Are you sure `__builtin_expect` is helping here? If so, it may need to be wrapped in `#ifdef`s. 


================
Comment at: include/atomic:1533
+    auto * const __c = __libcpp_contention_state(__a);
+    __cxx_atomic_thread_fence(memory_order_seq_cst);
+    if (0 != __cxx_atomic_load(__cxx_atomic_rebind(&__c->__waiters), memory_order_relaxed))
----------------
`memory_order_seq_cst`  is depricated, use `memory_order::seq_cst`.


================
Comment at: include/atomic:1534
+    __cxx_atomic_thread_fence(memory_order_seq_cst);
+    if (0 != __cxx_atomic_load(__cxx_atomic_rebind(&__c->__waiters), memory_order_relaxed))
+#endif
----------------
Ditto. 


================
Comment at: include/atomic:1536
+#endif
+        __libcpp_platform_wake((_Tp*)__a, true);
+}
----------------
Is this c-style cast OK? 


================
Comment at: include/atomic:1585
+template <class _Tp>
+_LIBCPP_INLINE_VISIBILITY void __cxx_atomic_notify_one(__cxx_atomic_impl<_Tp> const volatile*) {
+}
----------------
This will just silently fail if `_LIBCPP_HAS_NO_THREAD_CONTENTION_TABLE`. Is that OK?


================
Comment at: include/atomic:1597
+#ifndef _LIBCPP_NO_SPINNING
+    for(int __i = 0; __i < 16; ++__i) {
+        if(!__cxx_atomic_compare_equal(__cxx_atomic_load(__a, __order), __val))
----------------
I'm curious, why `16` here? 


================
Comment at: include/atomic:1690
 
+    _LIBCPP_INLINE_VISIBILITY void wait(_Tp __v, memory_order __m = memory_order_seq_cst) const volatile _NOEXCEPT
+        {__cxx_atomic_wait(&__a_, __v, __m);}
----------------
Use the new memory orders here. 


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480