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

[JF Bastien via Phabricator]

jfb added a comment.

One downside to forwarding layers is that if you don't have an always inline attribute, some build might not inline. This leads to code bloat and crappy codegen because the `memory_order` attribute is no longer a known `constexpr` value. Maybe we should have always inline?

You don't define `LIBCXX_FREESTANDING` yet, right?

I think at a high level this looks fine.



================
Comment at: libcxx/include/atomic:651
     __gcc_atomic::__can_assign<volatile _Atomic(_Tp)*, _Tp>::value>::type
-__c11_atomic_init(volatile _Atomic(_Tp)* __a,  _Tp __val) {
+__cxx_atomic_init(volatile _Atomic(_Tp)* __a,  _Tp __val) {
   __a->__a_value = __val;
----------------
IIRC this `__c11_atomic_*` code is only there under `#if defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)`, which works around limitations when you don't have the compiler builtins? clang otherwise defines them in `include/clang/Basic/Builtins.def`. Have you tried this out on GCC to make sure it works as expected?

FWIW the docs are here: https://clang.llvm.org/docs/LanguageExtensions.html#c11-atomic-builtins

I think the forwarding layer looks fine, just want to make sure we're on the same page.


================
Comment at: libcxx/include/atomic:857
+
+#else
+
----------------
Can you add a comment on what `#if` the `#else` corresponds to?


================
Comment at: libcxx/include/atomic:1062
+    template<typename _Tp> struct __is_always_lock_free<_Tp*> { enum { __value = 2 == ATOMIC_POINTER_LOCK_FREE }; };
+    template<> struct __is_always_lock_free<nullptr_t> { enum { __value = 2 == ATOMIC_POINTER_LOCK_FREE }; };
+#endif
----------------
Fine code indeed 😉


================
Comment at: libcxx/include/atomic:1074
+      "std::atomic<Tp> requires that 'Tp' be a trivially copyable type");
+#endif
+
----------------
Does libcxx have a workaround for this? I know LLVM and Chrome do...


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56913