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

Mon Jun 7 03:17:10 PDT 2021

tambre added a comment.

Thanks for the lightning fast review!

================
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","* *",""
----------------
Quuxplusone wrote:
> 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.)
> You also need to change the value of `__cpp_lib_atomic_value_initialization` and re-run `libcxx/utils/generate_feature_test_macro_components.py`.

Added.

> 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

[[ https://github.com/llvm/llvm-project/commit/06aaf0b3431f29b6debbb96fdd92ada896f336ff | Commit 06aaf0b3431f29b6debbb96fdd92ada896f336ff ]] adjusted the synopsis "to match what is implemented" so I assumed that part was considered done.
The required change seems to be simply adding `_LIBCPP_DEPRECATED_IN_CXX20` so I don't think it's worthy of a separate change. I've added it here.

================
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:
> 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.

================
Comment at: libcxx/include/atomic:1679

+#if _LIBCPP_STD_VER >= 20
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
----------------
Quuxplusone wrote:
> `> 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.)
Done. Thanks for the background on why it should be used like that.

================
Comment at: libcxx/include/atomic:1714
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    __atomic_base() _NOEXCEPT : __base{} {}
+#else
----------------
Quuxplusone wrote:
> 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).
Changed to the parentheses variant.
`T()` and `T{}` are equivalent since they're both variants of value initialization.
`T(args)` and `T{args}` (direct initialization) should be equivalent for aggregates since C++20 as a result of P0960R3 (except in case of narrowing).

> 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

I'm not sure either. P1008R1 did change this to not be aggregate initialization when using `{}`.

================
Comment at: libcxx/include/atomic:1714
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    __atomic_base() _NOEXCEPT : __base{} {}
+#else
----------------
tambre wrote:
> Quuxplusone wrote:
> > 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).
> Changed to the parentheses variant.
> `T()` and `T{}` are equivalent since they're both variants of value initialization.
> `T(args)` and `T{args}` (direct initialization) should be equivalent for aggregates since C++20 as a result of P0960R3 (except in case of narrowing).
> 
> > 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
> 
> I'm not sure either. P1008R1 did change this to not be aggregate initialization when using `{}`.
> 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>();
----------------
Quuxplusone wrote:
> `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?
> `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.

Done.

> Also please test `test<std::atomic<bool>>()`, since it looks like it gets a different specialization?

I think this should be covered by the `trivial` variant since the general case is used if "integral and not `bool`", but I've added it.

> Also nit: `> >` is a C++03-ism, and we can use `>>` here.

Fixed. I trusted `clang-format` here. This is probably the result of `Standard: Cpp03` in libcxx's `.clang-format`. Had to use `--nolint` for `arc diff`.

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

Not sure, I've added an empty body. `is_nothrow_constructible.pass.cpp` similarly has many functions without bodies.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103769