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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D56913#1386593 <https://reviews.llvm.org/D56913#1386593>, @__simt__ wrote:

> I will come back with another patch that addresses these comments.


Great! I'll be away until next Monday, so don't expect replies from me until then.

> Also will start a thread to debate C++03 support.

Good, thanks!

Also, I'm still looking for other maintainers to take a look, especially at the part that removes `__c11_atomic_xxx` functions from the global namespace and removes the `#define _Atomic(_Tp)`. I like those changes, just want to make sure it works for everybody.



================
Comment at: libcxx/include/atomic:557
+    !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP) && \
+    !defined(_LIBCPP_HAS_EXTERNAL_ATOMIC_IMP)
 #error <atomic> is not implemented
----------------
__simt__ wrote:
> ldionne wrote:
> > 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++).
> Actually this has been good for me.
> 
> First, there is `_LIBCPP_HAS_THREAD_API_EXTERNAL`, which I've had to set for CUDA in RTC mode. This is analogous to that, in that you don't know what the underlying support is, but something has  vouched that it conforms to the contract with the header.
> 
> Second, this allows a back-end that isn't GCC or C11 to slide underneath <atomic>. In that case, <atomic> does nothing to help, it assumes outright that the `__cxx_atomic...` symbols exist. That's the contract.
> 
> I would like to keep this one, but like anything else, I can keep it in the downstream.
Seeing the precedent for `_LIBCPP_HAS_THREAD_API_EXTERNAL`, I am comfortable with keeping this. Ideally we'd have a design document for `<atomic>` like we do for `<thread>` (`docs/DesignDocs/ThreadingSupportAPI.rst`), but I'm not putting that on you.


================
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); 
----------------
__simt__ wrote:
> ldionne wrote:
> > I think the first parameter should be `__cxx_atomic_type<_Tp*>`, but then that function already exists above.
> It exists in another #ifdef, right? I could probably use the `using` name here, if it mattered. This path is the C11 path, though, so `_Atomic` is what we're looking to match.
> 
> FWIW, template type deduction involving `_Atomic(_Tp)` is terrible.
I don't understand this. You (rightly) removed all functions of the form `__cxx_atomic_xxx(_Atomic(_Tp), ...)` in favour of `__cxx_atomic(__cxx_atomic_type<_Tp*>, ...)` but you only left this one. I don't understand why this one is still here.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D56913