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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for working this. (Actually I was considering to look at it, but happy I didn't start ;-))
Can you investigate the CI errors that are introduced with this patch?



================
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","* *",""
----------------
Can you add a note that floating-point and `shared_ptr` aren't done yet?


================
Comment at: libcxx/include/atomic:513
 {
-    atomic_flag() noexcept = default;
+    constexpr atomic_flag() noexcept = default; // constexpr since C++20
     atomic_flag(const atomic_flag&) = delete;
----------------
Likewise I prefer an until and since version.


================
Comment at: libcxx/include/atomic:1802
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    atomic() _NOEXCEPT_(is_nothrow_default_constructible_v<_Tp>) : __base() {}
+#else
----------------
Since this code is version guarded you can use `constexpr` and `noexcept` directly.


================
Comment at: libcxx/include/atomic:2566
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    atomic_flag() _NOEXCEPT : __a_() {}
+#else
----------------
Quuxplusone wrote:
> Since `__a_` is not literally `bool`, I would feel slightly safer if this explicitly said `__a_(false)`; compare line 2573. Sorry for not noticing this the first time around.
Can you update the test `test/std/atomics/atomics.flag/default.pass.cpp` to validate the new behaviour?


================
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;
----------------
Quuxplusone wrote:
> tambre wrote:
> > Quuxplusone wrote:
> > > 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).
> > Done.
> Nit: I think the leading `constexpr` should be dropped:
> ```
> atomic() noexcept;  // constexpr since C++20
> ```
> but if anyone disagrees, I don't care that much. These synopses are all pretty ad-hoc. :)
> 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).
I think it would be better to show the difference. (Actually I already wrote a comment about it, only to discover these remarks were a line below.)




================
Comment at: libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp:29
+
+int main() {
+  static_assert(test<std::atomic<bool>>());
----------------
Since the test is compile-time only there's no need for a run-time check. Can you make the following changes
- rename the file to `constexpr_noexcept.compile.pass.cpp`
- `int main()` -> `void test()`
- Add the following two lines at the end:
```
// Required for MSVC internal test runner compatibility.
int main(int, char**) { return 0; }
```


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/constexpr_noexcept.pass.cpp:30
+int main() {
+  static_assert(test<std::atomic<bool>>());
+  static_assert(test<std::atomic<int>>());
----------------
We typically use the macros `ASSERT_NOEXCEPT` and `ASSERT_NOT_NOEXCEPT` from "test_macros.h".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103769