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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF 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;
+}
----------------
This doesn't seem like an implementation of an atomic comparison. 


================
Comment at: include/atomic:1464
+template<typename _Tp = int>
+__cxx_atomic_impl<_Tp>* __cxx_atomic_rebind(_Tp* __inst) {
+    static_assert(sizeof(__cxx_atomic_impl<_Tp>) == sizeof(_Tp),"");
----------------
I'm not sure I'm comfortable with this atomic type-punning.


================
Comment at: include/atomic:2791
 
+static_assert(ATOMIC_INT_LOCK_FREE, "This library assumes atomic<int> is lock-free.");
+
----------------
I'm pretty sure we're still required to compile even if `int` doesn't have lock-free atomic operations.

Shouldn't we just omit the typedefs?


================
Comment at: include/barrier:46
+
+#ifndef __simt__
+#include <__config>
----------------
What's supposed to happen if `__simt__` is defined?


================
Comment at: include/barrier:67
+
+struct EmptyCompletionF 
+{
----------------
Reserved name?


================
Comment at: include/barrier:91
+    };
+    ::std::vector<__state_t>   state;
+
----------------
`vector` should resolve to the correct name. No need to qualify it.


================
Comment at: include/barrier:94
+    inline _LIBCPP_INLINE_VISIBILITY
+    bool __arrive(__phase_t const old_phase)
+    {
----------------
`old_phase`, `half`, and friends all need to be reserved names.


================
Comment at: include/barrier:105
+        for(size_t round = 0;; ++round) {
+            assert(round <= 63);
+            if(current_expected == 1)
----------------
The library doesn't use `assert` internally.


================
Comment at: include/cstddef:63
 #if _LIBCPP_STD_VER > 14
+#ifdef _LIBCPP_BEGIN_NAMESPACE_STD_NOVERSION
+_LIBCPP_BEGIN_NAMESPACE_STD_NOVERSION
----------------
I don't think these changes are meant to be here.


================
Comment at: include/latch:60
+{
+    alignas(64) atomic<ptrdiff_t> counter;
+public:
----------------
What's with all the explicit alignment specifications?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D68480