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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a reviewer: __simt__.
ldionne added a comment.

Thanks for working on that! I left some general comments, however I believe the best person to give you an in-depth review about this is @__simt__, as a co-author of the paper and as being the de-facto owner of `<atomic>`.

Misc note: Please update `libcxx/www/cxx2a_status.html`.



================
Comment at: libcxx/include/atomic:139
+template <class T>
+struct atomic_ref
+{
----------------
`// since C++20` (applies below too)


================
Comment at: libcxx/include/atomic:157
+
+    explicit atomic_ref(T& obj) noexcept;
+    atomic_ref(const atomic_ref&) noexcept;
----------------
It is customary to put these members closer to the top.


================
Comment at: libcxx/include/atomic:808
+
+template<typename Tp_, template<typename> class TemplateTp_>
+struct __is_instantiation_of : false_type { };
----------------
Reserved identifiers are of the form `_Foo`, not `Foo_`. You'll need to make that (simple) change throughout.


================
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,
----------------
What is preventing you from adding this check on Clang today?


================
Comment at: libcxx/include/atomic:1829
+template <class _Tp>
+struct atomic_ref
+  : public __atomic_ref_base<_Tp>
----------------
You should guard this with `#if _LIBCPP_STD_VER > 17` (applies below too).


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp:138
+}
+
+/*
----------------
I believe it would be better to add new test files just for `atomic_ref`.


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/trivially_copyable_ref.fail.cpp:54
+int main(int, char**) {
+  test(NotTriviallyCopyable(42));
+
----------------
How do you expect it to fail? I would strongly suggest using a `.verify.cpp` test instead and pinning down the error you're expecting to see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72240