[libcxx-commits] [PATCH] D103769: [libcxx] Implement P0883R2 ("Fixing Atomic Initialization")

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

LGTM % comments, but several comments are major.



================
Comment at: libcxx/docs/Cxx2aStatusPaperStatus.csv:136
 "","","","","",""
-"`P0883 <https://wg21.link/P0883>`__","LWG","Fixing Atomic Initialization","Belfast","* *",""
+"`P0883 <https://wg21.link/P0883>`__","LWG","Fixing Atomic Initialization","Belfast","|Complete|","13.0"
 "`P1391 <https://wg21.link/P1391>`__","LWG","Range constructor for std::string_view","Belfast","* *",""
----------------
You also need to change the value of `__cpp_lib_atomic_value_initialization` and re-run `libcxx/utils/generate_feature_test_macro_components.py`.

Also, p0883r2 asks to deprecate `atomic_init`. That could easily be a separate PR, but I don't think we should mark p0883 "Complete" until that's complete.
http://eel.is/c++draft/depr.atomics.general

(p0883 is also supposed to affect `atomic<shared_ptr<T>>` and `atomic<weak_ptr<T>>`, but libc++ does not implement those specializations (yet). So I think that part is OK.)


================
Comment at: libcxx/include/atomic:205-206
 
-    atomic() noexcept = default;
+    atomic() noexcept = default; // until C++17
+    constexpr atomic() noexcept; // since C++17
     constexpr atomic(T* desr) noexcept;
----------------
Nit: IMHO you could/should make this
```
atomic() noexcept;  // constexpr since C++20
```
and likewise below. (I'm guessing this one should say "C++20" like the others, too.)
But I do see that C++20 dropped the `=default`, and maybe that's significant enough to call out in the synopsis (but IMHO it's not).


================
Comment at: libcxx/include/atomic:1679
 
+#if _LIBCPP_STD_VER >= 20
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
----------------
`> 17`. (C++2b behaves more like 23 than like 20. We don't //actually// expect to see e.g. `18` here anymore, but if we did, it'd mean C++2a, which should behave more like 20 than like 17. This is a general libc++ style no matter which three-year cycle we're talking about.)


================
Comment at: libcxx/include/atomic:1714
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    __atomic_base() _NOEXCEPT : __base{} {}
+#else
----------------
Personally I'd prefer to see `__base()` here. The Standard says `T()` not `T{}`:
http://eel.is/c++draft/atomics.types.operations#2
In fact, don't they have visibly different effects when `T` is `struct A { int i; A() = default; }`? Or is that different in C++20 because aggregate init, and/or different everywhere now due to some DR? I'm having trouble demonstrating the issue on Godbolt: https://godbolt.org/z/o5aY7E3Eb
I //can// demonstrate that `A a;` and `auto a = A();` have different behavior (but I don't actually understand why): https://godbolt.org/z/3r75zG8Ms

Ditto below (line 1804 and anywhere I missed).


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp:29-32
+  test<std::atomic<int> >();
+  test<std::atomic<int*> >();
+  test<std::atomic<trivial> >();
+  test<std::atomic_flag>();
----------------
`static_assert(test<std::atomic<int>>());` etc., and make the constexpr function `test` end with `return true;`. This way you make sure it's actually happening at compile time.
Also please test `test<std::atomic<bool>>()`, since it looks like it gets a different specialization?
Also nit: `> >` is a C++03-ism, and we can use `>>` here.

Also, doesn't the constructor of `throwing` need a body? How does this link?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103769