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

[Benjamin Trapani via Phabricator via libcxx-commits]

BenjaminTrapani 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);
 }
----------------
__simt__ wrote:
> 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.
Sorry for the delay in responding to all these comments. I will fix per these comments today. For the purposes of this change, is it ok to leave atomic_ref unimplemented for the C11 path, and look into removing the C11 path later?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72240