[libcxx-commits] [PATCH] D112221: Mark ATOMIC_VAR_INIT and ATOMIC_FLAG_INIT as deprecated

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

Quuxplusone added a comment.

> I don't know enough about libc++ to feel comfortable making those changes yet. For example, does libc++ need to work with other compilers than Clang (compilers which might give diagnostics if you fail to use ATOMIC_VAR_INIT in some language mode)? The deprecation is not really a DR, so should the uses be wrapped in language version checks, etc. Or are you saying I don't have to worry about any of that and I can rip this stuff out of libc++ with wild abandon?

@ldionne will be the ultimate arbiter, but FWIW, //I think// you can rip with wild abandon — certainly from `libcxx/src/`.
For some background (and further links, including p0883 which you've already dug up), see
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2139r2.html#3.24

There are two kinds of places libc++ uses these macros: `src/` and `test/`. Everywhere in `src/` is compiled with C++17, so it's totally safe to use `atomic<T> a{init};` instead of `atomic<T> a = ATOMIC_VAR_INIT(init);`.
But everywhere in `test/` we need to keep working, by definition. They might have to gain a line `ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS` or (new and therefore worse) `ADDITIONAL_COMPILE_FLAGS: -Wno-deprecated`, but that's all.

Here's the diffs in `src/` AFAICT [untested code]:

  diff --git a/libcxx/src/barrier.cpp b/libcxx/src/barrier.cpp
  index 9ee476993b81..8e13ad088052 100644
  --- a/libcxx/src/barrier.cpp
  +++ b/libcxx/src/barrier.cpp
  @@ -22,7 +22,7 @@ public:
       struct alignas(64) /* naturally-align the heap state */ __state_t
       {
           struct {
  -            __atomic_base<__barrier_phase_t> __phase = ATOMIC_VAR_INIT(0);
  +            __atomic_base<__barrier_phase_t> __phase{0};
           } __tickets[64];
       };
   
  diff --git a/libcxx/src/experimental/memory_resource.cpp b/libcxx/src/experimental/memory_resource.cpp
  index 5f9d3b8a4cbe..77a1d654d60f 100644
  --- a/libcxx/src/experimental/memory_resource.cpp
  +++ b/libcxx/src/experimental/memory_resource.cpp
  @@ -97,8 +97,7 @@ static memory_resource *
   __default_memory_resource(bool set = false, memory_resource * new_res = nullptr) noexcept
   {
   #ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
  -    _LIBCPP_SAFE_STATIC static atomic<memory_resource*> __res =
  -        ATOMIC_VAR_INIT(&res_init.resources.new_delete_res);
  +    _LIBCPP_SAFE_STATIC static atomic<memory_resource*> __res{&res_init.resources.new_delete_res};
       if (set) {
           new_res = new_res ? new_res : new_delete_resource();
           // TODO: Can a weaker ordering be used?
  diff --git a/libcxx/src/ios.cpp b/libcxx/src/ios.cpp
  index a8a99015a977..338636b2220e 100644
  --- a/libcxx/src/ios.cpp
  +++ b/libcxx/src/ios.cpp
  @@ -137,7 +137,7 @@ ios_base::getloc() const
   
   // xalloc
   #if defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_NO_THREADS)
  -atomic<int> ios_base::__xindex_ = ATOMIC_VAR_INIT(0);
  +atomic<int> ios_base::__xindex_{0};
   #else
   int ios_base::__xindex_ = 0;
   #endif
  diff --git a/libcxx/test/std/thread/futures/futures.async/async.pass.cpp b/libcxx/test/std/thread/futures/futures.async/async.pass.cpp
  index 365b895e0603..beee6f8fcfdd 100644
  --- a/libcxx/test/std/thread/futures/futures.async/async.pass.cpp
  +++ b/libcxx/test/std/thread/futures/futures.async/async.pass.cpp
  @@ -33,7 +33,7 @@
   typedef std::chrono::high_resolution_clock Clock;
   typedef std::chrono::milliseconds ms;
   
  -std::atomic_bool invoked = ATOMIC_VAR_INIT(false);
  +std::atomic_bool invoked{false};
   
   int f0()
   {



================
Comment at: clang/lib/Headers/stdatomic.h:42-47
 #define ATOMIC_VAR_INIT(value) (value)
+#if __STDC_VERSION__ >= 201710L || __cplusplus >= 202002L
+/* ATOMIC_VAR_INIT was deprecated in C17 and C++20. */
+#pragma clang deprecated(ATOMIC_VAR_INIT)
+#endif
 #define atomic_init __c11_atomic_init
----------------
Hmm, I do think there ought to be some way for the C++20 programmer to suppress the deprecation warning for these macros (specifically, not just via `-Wno-deprecated` in their whole project). For deprecations in the C++ standard library, libc++ offers an all-or-nothing flag: basically you'd do
```
#if (__STDC_VERSION__ >= 201710L || __cplusplus >= 202002L) && !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS)
/* ATOMIC_VAR_INIT was deprecated in C17 and C++20. */
#pragma clang deprecated(ATOMIC_VAR_INIT)
#endif
```
(This also makes it easy for libcxx/test/ to suppress the deprecation warning for the purposes of testing.)

However, I'm not sure if it's appropriate to mention `_LIBCPP_DISABLE_DEPRECATION_WARNINGS` in this header located in clang/lib/Headers/ instead of libcxx/include/. Someone else will have to make that call.

It might be that the only way for the programmer (or libcxx/test/) to work around the warning will be for them to pass `-Wno-deprecated` globally; IMO that is suboptimal but quite far from disastrous.


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

https://reviews.llvm.org/D112221