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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/atomic:896
+template<typename _Tp, typename _Sco>
+struct __cxx_locked_atomic_type {
+
----------------
Why is this not in `std::`?


================
Comment at: libcxx/include/atomic:929
+static _LIBCPP_INLINE_VISIBILITY inline 
+void __c11_atomic_init(volatile __cxx_locked_atomic_type<_Tp, _Sco>* __a,  _Tp __val) {
+  __a->__a_value = __val;
----------------
It seems wrong to have those be named `__c11_atomic_xxx` since they have nothing to do with C11. Instead, the code in `atomic` and `__atomic_base` could make calls to functions called `__atomic_xxx_impl(...)`, and that could be implemented as C11 atomics in one case and as your `__cxx_locked_atomic_type` in another case.

So basically, instead of introducing this new version of `__c11_atomic_init`, rename the existing one to `__atomic_init_impl`, and then add your version under the name `__atomic_init_impl` too. What do you think? If you agree this would yield more clarity, we could apply this rename in a separate patch prior to this one.

I don't think the rename would be an ABI break because all those functions are `static inline` today, so they have internal linkage anyway (and hence they exist in each TU where they're used).


================
Comment at: libcxx/include/atomic:1045
+template <class _Tp, class _Sco>
+using __atomic_type = typename conditional<__atomic_always_lock_free(sizeof(_Tp), 0), 
+                                           _Atomic_s(_Tp, _Sco),
----------------
Not as cute, but you could use a traditional metafunction instead of an alias to make this potentially work in C++03. Not that I'm attached to supporting atomics in C++03, but it seems like we do (looking at the tests).


================
Comment at: libcxx/include/atomic:1045
+template <class _Tp, class _Sco>
+using __atomic_type = typename conditional<__atomic_always_lock_free(sizeof(_Tp), 0), 
+                                           _Atomic_s(_Tp, _Sco),
----------------
ldionne wrote:
> Not as cute, but you could use a traditional metafunction instead of an alias to make this potentially work in C++03. Not that I'm attached to supporting atomics in C++03, but it seems like we do (looking at the tests).
Also, I think this layer is interesting because it marks the boundary between implementation and interface. Basically, `atomic` and `__atomic_base` are both used to provide the interface that's mandated by the standard. `__atomic_type` is where the actual implementation begins, be it through `_Atomic` or the `__cxx_locked_atomic_type` you're introducing.

I believe that making this explicit in the name of the type might add clarity, for example `__atomic_impl`. WDYT?


================
Comment at: libcxx/include/atomic:1059
+#ifndef _LIBCPP_ATOMIC_SCOPE_DEFAULT
+#define _LIBCPP_ATOMIC_SCOPE_DEFAULT int
+#endif
----------------
Can you explain what this scope is?


================
Comment at: libcxx/include/atomic:1063
+template <class _Tp, class _Sco = _LIBCPP_ATOMIC_SCOPE_DEFAULT, bool = is_integral<_Tp>::value && !is_same<_Tp, bool>::value>
 struct __atomic_base  // false
 {
----------------
I don't think adding the `_Sco` template parameter here is an issue from an ABI stand point, because I don't think `__atomic_base` ever gets exposed at an ABI boundary. At the very least, we don't export any symbol in libc++ that contains this type. But I could be wrong so I'd like someone else to double-check this.


================
Comment at: libcxx/include/atomic:1265
     _LIBCPP_INLINE_VISIBILITY
-    atomic() _NOEXCEPT _LIBCPP_DEFAULT
+#ifndef _LIBCPP_CXX03_LANG
+    atomic() _NOEXCEPT = default;
----------------
__simt__ wrote:
> jfb wrote:
> > Do we support `atomic` in C++03 mode?
> I couldn't get this to build fully successfully without -std=c++11.
> 
> Here my change is for consistency, this is what's done on other objects in this file.
I'm confused as to why this is needed. It seems like just `atomic() _NOEXCEPT _LIBCPP_DEFAULT` should still work fine, as even in the case where `_LIBCPP_DEFAULT` expands to `{}`, we get:

```
atomic() _NOEXCEPT {}
```

But that should default-construct `__base` as expected. Do you mean that you tried building in C++03 mode without this change and the tests didn't pass? That sure is surprising. If that's the case, then leave as-is, but otherwise if it's just for consistency then I would stick with what's there.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56913