[libcxx-commits] [PATCH] D72240: Implement C++20 std::atomic_ref and test

[Olivier Giroux via Phabricator via libcxx-commits]

__simt__ added inline comments.


================
Comment at: libcxx/include/atomic:1015
+__cxx_atomic_pointer_to_data(_Tp* __value) {
+  return reinterpret_cast<typename __annotate_with_qualifiers<_Atomic(typename __cxx_atomic_base_impl_traits<_Tp>::__underlying_t), _Tp>::__type*>(&__value->__a_value);
 }
----------------
rsmith wrote:
> I find this cast concerning. There's no guarantee that `_Atomic(T)` is compatible with plain `T` in general (they might have different representations), and even if the layout is identical, I don't think Clang's strict aliasing rules provide a guarantee that you can access a `T` via an `_Atomic(T)`. (We happen to treat atomic types as can-alias-anything right now, but that's only because we've not implemented strict aliasing for atomics yet.)
> 
> Can we use the implementation in terms of `__atomic_*` for `atomic_ref` instead? Those builtins are designed to be used on objects that are declared with type `T` rather than type `_Atomic(T)`.
I agree with you there. It looks like `atomic_ref<>` is not implementable (at least not at high QoI) on top of C11 the way that `atomic<>` was.

I do wish the `_Atomic(T)` path simply wasn't there in libcxx, and we just always used the `__atomic_*` path but the Mac build chooses the `_Atomic(T)` path. At least it does today.

Not sure how to reconcile that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72240