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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/atomic:1829
+template <class _Tp>
+struct atomic_ref
+  : public __atomic_ref_base<_Tp>
----------------
__simt__ wrote:
> 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.
Ok. I guess we've been down that road already anyway.


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp:138
+}
+
+/*
----------------
__simt__ wrote:
> 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.
As you prefer. I imagine it might look like some generic test that works with `atomic` *and* `atomic_ref`. If so, it's easy to have the generic test in a header file and then instantiate it once from a `atomic.pass.cpp` file and once from a `atomic_ref.pass.cpp`. This would keep the test structure consistent to what it is today.

But I am not strongly attached to a particular approach -- whatever's more convenient works, I was just trying to retain some consistency with the way we've been doing things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72240