[libcxx-commits] [PATCH] D56913: decoupling Freestanding atomic<T> from libatomic.a

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Here's a summary of what I understand the current state of the patch is (mostly for my own reference as a bird's eye view):

  #if defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
  
      template <typename _Tp>
      struct __cxx_atomic_type {
          // Implementation using a raw _Tp and GCC's __atomic_xxx operations
      };
  
      template<typename _Tp>
      using __cxx_atomic_base_impl = __cxx_atomic_type<_Tp>;
  
  #elif defined(_LIBCPP_HAS_C_ATOMIC_IMP)
  
      template <typename _Tp>
      struct __cxx_atomic_type {
          // Implementation using an _Atomic(_Tp) and the __c11_atomic_xxx operations
      };
  
      template<typename _Tp>
      using __cxx_atomic_base_impl = __cxx_atomic_type<_Tp>;
  
  #endif
  
  
  #if defined(_LIBCPP_FREESTANDING) && defined(__cpp_lib_atomic_is_always_lock_free)
  
      template<typename _Tp>
      struct __cxx_atomic_lock_impl {
          // Implementation using a raw _Tp and a lock implemented as a __cxx_atomic_base_impl
      };
  
      template <class _Tp>
      using __cxx_atomic_impl = _Tp is always lock free ? __cxx_atomic_base_impl<_Tp> : __cxx_atomic_lock_impl<_Tp>;
  
  #else
  
      template <class _Tp>
      using __cxx_atomic_impl = __cxx_atomic_base_impl<_Tp>;
  
  #endif
  
  template <class _Tp>
  struct __atomic_base {
      // Implements the interface of <atomic> and uses a __cxx_atomic_impl<_Tp> under the hood
  };

Would it make sense to decide whether we want to use GCC's non-lockfree atomics or not based on a configuration macro that's not `_LIBCPP_FREESTANDING`?



================
Comment at: libcxx/include/atomic:555
 #endif
-#if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
+#if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && \
+    !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP) && \
----------------
Wouldn't it be simpler to say:

```
#if defined(_LIBCPP_HAS_NO_ATOMIC_HEADER)
#  error <atomic> is not implemented
#endif
```


================
Comment at: libcxx/include/atomic:592
+
+template <typename _Tp> _Tp const& __create();
+template <typename _Tp> _Tp& __use();
----------------
I think uses of this can be replaced by `declval<_Tp const&>()`?


================
Comment at: libcxx/include/atomic:602
+  static const bool value =
+      sizeof(__test_atomic_assignable<_Tp, _Td>(1)) == sizeof(char);
+};
----------------
Would `std::is_assignable<_Td&, _Tp const&>` work too instead? Those traits are available even in C++03 mode.


================
Comment at: libcxx/include/atomic:641-642
+  char* from = reinterpret_cast<char*>(&__val);
+  while (to != end)
+    *to++ = *from++;
+}
----------------
Can you use `_VSTD::copy(to, end, from)` instead? Also, you should mangle the names to `__to`, `__end` and `__from`.

I admit that having to include `<algorithm>` here sucks, but at least please lift this definition into a function you can reuse below. In the future we should simply put `copy` and other basic algorithms in a header like `<__algorithm_base>` that can be used in places where we don't need all of `<algorithm>`.

EDIT: Nevermind this: I just saw this was a pre-existing condition in the code. Keeping the comment here to point out this is not great, but you don't have to fix it in this patch.


================
Comment at: libcxx/include/atomic:718
 template <typename _Tp>
-struct __gcc_atomic_t {
+struct __cxx_atomic_type {
 
----------------
I'm not seeing the value of this type not being called `__cxx_atomic_base_impl` directly. Is there any reason for this?


================
Comment at: libcxx/include/atomic:973
+template <typename _Tp>
+struct __cxx_atomic_type {
+
----------------
Likewise, this type could be called `__cxx_atomic_base_impl` directly?


================
Comment at: libcxx/include/atomic:1146
+
 #endif // _LIBCPP_HAS_GCC_ATOMIC_IMP
 
----------------
This `#endif` doesn't look right anymore.


================
Comment at: libcxx/include/atomic:1199
+
+  _LIBCPP_INLINE_VISIBILITY _Tp reader() const volatile {
+    _Tp __old;
----------------
`__reader` & friends should be mangled.


================
Comment at: libcxx/include/atomic:1215
+  _LIBCPP_INLINE_VISIBILITY void writer(_Function && __f) volatile {
+    while(1 == __cxx_atomic_exchange(&__a_lock, _LIBCPP_ATOMIC_FLAG_TYPE(true), memory_order_acquire));
+    __f(__a_value);
----------------
Can you please put the semi-colon on its own line, like so:

```
while (1 == __cxx_atomic_exchange(...))
    /* spin */;
```

or something like that. Just to make it obvious we're not calling `__f` in the loop. This applies elsewhere too.



Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56913