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

[Benjamin Trapani via Phabricator via libcxx-commits]

BenjaminTrapani marked 6 inline comments as done.
BenjaminTrapani added inline comments.


================
Comment at: libcxx/include/atomic:1501
+// TODO perform this check when compiled with clang too.
+#if _GNUC_VER >= 501
+    static_assert(is_trivially_copyable<_Tp>::value,
----------------
ldionne wrote:
> What is preventing you from adding this check on Clang today?
Should be fine to enable, although probably best done in another patch so we can revert if it breaks many things for a large clang user.


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp:138
+}
+
+/*
----------------
ldionne wrote:
> __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.
I thought about this a bit and the interfaces under test are substantially different. We could share the common parts by always constructing from lvalues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72240