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

[Olivier Giroux via Phabricator via libcxx-commits]

__simt__ added a comment.

We're looking at this patch in the context of the libcudacxx fork. I don't have more feedback right now but we probably will get there soon.



================
Comment at: libcxx/include/atomic:1829
+template <class _Tp>
+struct atomic_ref
+  : public __atomic_ref_base<_Tp>
----------------
ldionne wrote:
> You should guard this with `#if _LIBCPP_STD_VER > 17` (applies below too).
We will want to allow using this from C++11 onwards. If libcxx doesn't do that out of the box, then we'll be doing that in our private patchset.


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp:138
+}
+
+/*
----------------
ldionne wrote:
> I believe it would be better to add new test files just for `atomic_ref`.
Actually, I would prefer if we found a way to make the atomics tests cover atomic_ref, at breadth.

There is absolutely no way that this patch has sufficient test coverage right now, and duplicating all the tests is an inefficient use of our time (now and in the future).

For the most part, this could be tested by doing sufficiently clever refactoring in atomic_helpers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72240