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

Mon Jun 7 06:57:34 PDT 2021

Quuxplusone added inline comments.

================
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","* *",""
----------------
tambre wrote:
> 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.
> The required change seems to be simply adding `_LIBCPP_DEPRECATED_IN_CXX20`

Code change LGTM. We usually want to see a "deprecation warning test" added as well; I think you should add a `libcxx/test/libcxx/depr/depr.atomics.general/atomic_init.depr_in_cxx20.verify.cpp` along the same lines as `libcxx/test/libcxx/depr/depr.function.objects/adaptors.depr_in_cxx11.verify.cpp`. (My impression is that we're not very consistent about these tests, so I suggest copying-and-modifying `adaptors.depr_in_cxx11.verify.cpp` in the obvious way, but if @ldionne asks for some other testing strategy, you should listen to him not me.)

================
Comment at: libcxx/include/atomic:2566
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    atomic_flag() _NOEXCEPT : __a_() {}
+#else
----------------
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.

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

================
Comment at: libcxx/include/atomic:1714
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    __atomic_base() _NOEXCEPT : __base{} {}
+#else
----------------
tambre wrote:
> 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).
> 
> 
Bah, I missed the first time around that this is initializing `__atomic_base<_Tp, false> __base`, not `_Tp __a_`, anyway! The important parens-not-braces is on line 1679; I now agree it definitely doesn't matter which one we use here. Can't we eliminate this member-initializer entirely, because `__atomic_base<_Tp, false>`'s default ctor (line 1679) will do the right thing?  I think the whole `#if` can be removed, and all you need to change here is to add `_LIBCPP_CONSTEXPR_AFTER_CXX17`:
```
    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
    __atomic_base() _NOEXCEPT _LIBCPP_DEFAULT
```
Ditto lines 1800, 1832.

================
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>();
----------------
tambre wrote:
> 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.
Ah, I get it now: you instantiate `test<std::atomic<trivial>>` but not `test<std::atomic<throwing>>`. Instantiating `test<std::atomic<throwing>>` would require a body for `trivial`'s constructor.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103769