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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

I like where this is going generally speaking, but there's a couple of things I'd like to understand. It would also be great if other maintainers with more experience could chime in regarding the rename of `__c11_atomic_xxx` to `__cxx_atomic_xxx` -- is this removing functionality that we're providing (but we shouldn't be providing :-)?

There are several things in this patch what will not work in C++03. I guess we have to either:

- Have a discussion about whether `<atomic>` should support C++03. I vote NO, but I wasn't there when that choice was made and I don't know off the top of my head what the repercussions might be.
- Rewrite all of this patch in a way that works in C++03. It's possible everywhere (sometimes we might need to use an extension like `__typeof`), but it's going to be a pain in the neck.



================
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;
----------------
jfb wrote:
> 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.
I have the same concern -- I like where this patch is going and TBH I don't think that defining `_Atomic` and `__c11_atomic_xxx` operations in a user-facing way in libc++ is a good idea. But I want to understand what we're breaking by removing that.


================
Comment at: libcxx/include/atomic:557
+    !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP) && \
+    !defined(_LIBCPP_HAS_EXTERNAL_ATOMIC_IMP)
 #error <atomic> is not implemented
----------------
IIUC, it doesn't make sense for libc++ to have `_LIBCPP_HAS_EXTERNAL_ATOMIC_IMP` since it's a purely non-libc++ thing. Am I wrong? If not, please remove this knob for now and we can re-add it later on when we have a reason to (e.g. when/if we introduce scoped atomics into libc++).


================
Comment at: libcxx/include/atomic:588
+struct __cxx_atomic_scope { };
+#define _LIBCPP_ATOMIC_SCOPE_DEFAULT __cxx_atomic_scope
+#endif
----------------
Please get rid of scoped atomics for the moment since those are orthogonal to the rest of the patch.


================
Comment at: libcxx/include/atomic:658
 typename enable_if<
-    __gcc_atomic::__can_assign<volatile _Atomic(_Tp)*, _Tp>::value>::type
-__c11_atomic_init(volatile _Atomic(_Tp)* __a,  _Tp __val) {
+    __cxx_atomic_type::__can_assign<volatile __cxx_atomic_type<_Tp>*, _Tp>::value>::type
+__cxx_atomic_init(volatile __cxx_atomic_type<_Tp>* __a,  _Tp __val) {
----------------
This rename of `__gcc_atomic::__can_assign` to `__cxx_atomic_type::__can_assign` looks wrong -- `__cxx_atomic_type` is not a namespace. I don't think this will compile. I think that just `__can_assign` is what you need? Same comment applies below.


================
Comment at: libcxx/include/atomic:1049
+static _LIBCPP_INLINE_VISIBILITY inline 
+_Tp* __cxx_atomic_fetch_add(_Atomic(_Tp*) volatile* __a, ptrdiff_t __delta, int __order) {
+    return __c11_atomic_fetch_add(&__a->__a_value, __delta, __order); 
----------------
I think the first parameter should be `__cxx_atomic_type<_Tp*>`, but then that function already exists above.


================
Comment at: libcxx/include/atomic:1054
+static _LIBCPP_INLINE_VISIBILITY inline 
+_Tp* __cxx_atomic_fetch_add(_Atomic(_Tp*) * __a, ptrdiff_t __delta, int __order) {
+    return __c11_atomic_fetch_add(&__a->__a_value, __delta, __order); 
----------------
Ditto.


================
Comment at: libcxx/include/atomic:1092
 
+#if defined(_LIBCPP_FREESTANDING) && defined(__cpp_lib_atomic_is_always_lock_free)
+
----------------
Please implement `_LIBCPP_FREESTANDING` in `__config` as

```
#ifndef __STDC_HOSTED__
#  define _LIBCPP_FREESTANDING
#endif
```

This way, `_LIBCPP_FREESTANDING` will be defined whenever `-ffreestanding` is passed to Clang, which is the goal IIUC.


================
Comment at: libcxx/include/atomic:1109
+
+  _Tp __a_value;
+  mutable __cxx_atomic_base_impl<_LIBCPP_ATOMIC_FLAG_TYPE, _Sco> __a_lock;
----------------
Indentation looks weird here.


================
Comment at: libcxx/include/atomic:1135
+void __cxx_atomic_store(volatile __cxx_atomic_lock_impl<_Tp, _Sco>* __a,  _Tp __val, int) {
+  __a->writer_section([&](_Tp volatile& __a_value){ __a_value = __val; });
+}
----------------
None of those functions is going to work in C++03.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56913