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

[Olivier Giroux via Phabricator via libcxx-commits]

__simt__ marked 9 inline comments as done.
__simt__ added a comment.

> 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?

Sure it can make sense. That bridge is somewhere in my future, even. When I could do now is create another `_LIBCPP...` macro that gets set in the `__config` portion for atomics when `_LIBCPP_FREESTANDING` is set.



================
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) && \
----------------
ldionne wrote:
> Wouldn't it be simpler to say:
> 
> ```
> #if defined(_LIBCPP_HAS_NO_ATOMIC_HEADER)
> #  error <atomic> is not implemented
> #endif
> ```
Yes. I'll do that for the next one.


================
Comment at: libcxx/include/atomic:592
+
+template <typename _Tp> _Tp const& __create();
+template <typename _Tp> _Tp& __use();
----------------
ldionne wrote:
> I think uses of this can be replaced by `declval<_Tp const&>()`?
These are there for c++03 support, I think. Much of this section is borrowed from the previous version, but expanded to suit the additional uses in the file now.


================
Comment at: libcxx/include/atomic:602
+  static const bool value =
+      sizeof(__test_atomic_assignable<_Tp, _Td>(1)) == sizeof(char);
+};
----------------
ldionne wrote:
> Would `std::is_assignable<_Td&, _Tp const&>` work too instead? Those traits are available even in C++03 mode.
Are they? I can probably use them then. When I test in c++03 mode next, I'll experiment with the substitution.


================
Comment at: libcxx/include/atomic:641-642
+  char* from = reinterpret_cast<char*>(&__val);
+  while (to != end)
+    *to++ = *from++;
+}
----------------
ldionne wrote:
> 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.
Thanks for the EDIT... because the patch is trending towards fixing-all-the-things.

Also, introducing a dependency on `<algorithm>` is a problem for me right now. I'd like to avoid it for a while longer, or else create the `<__algorithm_base>` at that time.


================
Comment at: libcxx/include/atomic:718
 template <typename _Tp>
-struct __gcc_atomic_t {
+struct __cxx_atomic_type {
 
----------------
ldionne wrote:
> I'm not seeing the value of this type not being called `__cxx_atomic_base_impl` directly. Is there any reason for this?
This makes more sense in the scoped version. In this version I can do what you requested here, that's what I'll come back with.


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


================
Comment at: libcxx/include/atomic:1146
+
 #endif // _LIBCPP_HAS_GCC_ATOMIC_IMP
 
----------------
ldionne wrote:
> This `#endif` doesn't look right anymore.
Right. Will fix.


================
Comment at: libcxx/include/atomic:1199
+
+  _LIBCPP_INLINE_VISIBILITY _Tp reader() const volatile {
+    _Tp __old;
----------------
ldionne wrote:
> `__reader` & friends should be mangled.
Ok. But if we retain c++03 support then this is going to have to change. Or at least be guarded by another `#if` condition if we temporarily force c++11 support.


================
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);
----------------
ldionne wrote:
> 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.
> 
Will do.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56913