[libcxx] Add deprecation notices to macros deprecated in P0883R2

[tambre]

Summary

When P0883R2 was initially implemented in D103769 #pragma clang deprecated didn't exist yet.
We also forgot to cleanup usages in libc++ itself.

This takes care of both.

[tambre]

Should I bump the version listed for P0883R2 to 14.0? This only adds an extra non-mandated deprecation warning.

libcxx/include/atomic

2703

Separate macro deprecation macros didn't seem worth it as this is likely to be the only place we'd use them for a while. Since other compilers should simply ignore these guarding these shouldn't be necessary either.

[• Quuxplusone]

LGTM mod important comment.

libcxx/include/atomic

2703

Sadly "other compilers" includes "old Clang."

atomic:2703:15: error: unknown pragma ignored [-Werror,-Wunknown-pragmas]
#pragma clang deprecated(ATOMIC_FLAG_INIT)
              ^

So this should be guarded somehow.
Today, if I grepped correctly, libcxx has zero instances of any pragma clang or pragma GCC except for pragma {clang,GCC} diagnostic. So we're unlikely to have any precedent for how to deal with conditionally supported pragmas. I suggest just this:

#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS)
# if defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER >= 1400
#  pragma clang deprecated ~~~
# endif
#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS)

(Separating the "semantic, why" from the "compiler-specific, how" — but really I did that only so I wouldn't have to deal with a super-long line. ;))

[tambre]

Guard deprecation macros

libcxx/include/atomic

2703

Good point, done.

[ldionne]

In D115995#3201537, @tambre wrote:

Should I bump the version listed for P0883R2 to 14.0? This only adds an extra non-mandated deprecation warning.

Yes, I suggest we bump it so as to not advertise that we implemented a paper fully when we didn't.

LGTM but please update the status page. Thanks a lot for the patch!

[tambre]

Update paper status to version 14.0

[• Quuxplusone]

Okay, now that I'm hitting this in a real codebase, I'm stumped as to what the (WG21 and/or libc++) recommendation is, here. ATOMIC_FLAG_INIT is required up-to-and-including C++17, and then deprecated in-and-above C++20? So if I have code in my codebase today like

std::atomic_flag flag_ = ATOMIC_FLAG_INIT;

should I change it to

std::atomic_flag flag_ = {false};

or

std::atomic_flag flag_;

or what? How am I supposed to eliminate the deprecation warning? I've thought of

• stop allowing my codebase to compile in C++17 mode; require C++20 (this is a non-starter)
• write my own macro using #if __cplusplus >= 201700L
• turn on -Wno-deprecated in the build system
• just use = {false}; or ; on the assumption that I can file bugs against any library vendor that I catch doing the Wrong Thing in C++17 mode (but then I'd have to fall back to one of the other options anyway)

None of these options feel very nice.

This is the same situation we had with random_shuffle in C++11: https://stackoverflow.com/questions/27791474/what-are-best-practices-for-simple-random-shuffling-in-code-thats-both-c03-an

[tambre]

In D115995#3205319, @Quuxplusone wrote:

Okay, now that I'm hitting this in a real codebase, I'm stumped as to what the (WG21 and/or libc++) recommendation is, here. ATOMIC_FLAG_INIT is required up-to-and-including C++17, and then deprecated in-and-above C++20? So if I have code in my codebase today like

I don't see why you'd think using the macro would be required.

In D115995#3205319, @Quuxplusone wrote:

How am I supposed to eliminate the deprecation warning?

Explictly init to false. If C++20 and higher, then you can rely on the default constructor.

[• Quuxplusone]

In D115995#3205377, @tambre wrote:

In D115995#3205319, @Quuxplusone wrote:

Okay, now that I'm hitting this in a real codebase, I'm stumped as to what the (WG21 and/or libc++) recommendation is, here. ATOMIC_FLAG_INIT is required up-to-and-including C++17, and then deprecated in-and-above C++20? So if I have code in my codebase today like

I don't see why you'd think using the macro would be required.

AFAICT, on paper, C++17/20 atomic_flag has no constructors
https://en.cppreference.com/w/cpp/atomic/atomic_flag/atomic_flag
except for the default constructor (trivial in C++17) and the deleted copy constructor. So if you want an atomic_flag initialized with false, you need some way of getting false into it. On paper, there's no guarantee that std::atomic_flag f = {false}; will compile. But I just checked Godbolt and every major compiler does support that syntax, so I suppose it's safe to use in practice: https://godbolt.org/z/4cbbGcv96

Explicitly init to false. If C++20 and higher, then you can rely on the default constructor.

Yeah, the default ctor definitely works in C++20-and-above. Below C++20, the default ctor is on-paper "guaranteed" to be trivial and thus "guaranteed" not to work; but it looks like implementations largely ignore that, too, AFAICT, and just do the C++20 thing everywhere (plus the extra constructor from bool which is super helpful).

Anyway, I've added -Wno-deprecated-pragma to my build flags for now; that's not as terrible-feeling a workaround as I was worried it would be. And ultimately we'll probably just use the atomic_flag(bool) ctor, even though (AFAICT) it's technically an extension.

[tambre]

In D115995#3205826, @Quuxplusone wrote:

In D115995#3205377, @tambre wrote:

In D115995#3205319, @Quuxplusone wrote:

Okay, now that I'm hitting this in a real codebase, I'm stumped as to what the (WG21 and/or libc++) recommendation is, here. ATOMIC_FLAG_INIT is required up-to-and-including C++17, and then deprecated in-and-above C++20? So if I have code in my codebase today like

I don't see why you'd think using the macro would be required.

AFAICT, on paper, C++17/20 atomic_flag has no constructors
https://en.cppreference.com/w/cpp/atomic/atomic_flag/atomic_flag
except for the default constructor (trivial in C++17) and the deleted copy constructor. So if you want an atomic_flag initialized with false, you need some way of getting false into it. On paper, there's no guarantee that std::atomic_flag f = {false}; will compile. But I just checked Godbolt and every major compiler does support that syntax, so I suppose it's safe to use in practice: https://godbolt.org/z/4cbbGcv96

Ouch, you're correct. I was looking at std::atomic' not std::atomic_flag. Does seem like an oversight.

libcxx/include/atomic

 #endif
 
 typedef atomic<__libcpp_signed_lock_free> atomic_signed_lock_free;
 typedef atomic<__libcpp_unsigned_lock_free> atomic_unsigned_lock_free;
 
 #define ATOMIC_FLAG_INIT {false}
 #define ATOMIC_VAR_INIT(__v) {__v}
 
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS)
+# if defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER >= 1400
+#  pragma clang deprecated(ATOMIC_FLAG_INIT)
+#  pragma clang deprecated(ATOMIC_VAR_INIT)
+# endif
+#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS)
+
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP_ATOMIC

libcxx/src/barrier.cpp

 
 #if !defined(_LIBCPP_HAS_NO_TREE_BARRIER)
 
 class __barrier_algorithm_base {
 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];
     };
 
     ptrdiff_t&              __expected;
     unique_ptr<__state_t[]> __state;
 
     _LIBCPP_HIDDEN
     __barrier_algorithm_base(ptrdiff_t& __expected)

libcxx/src/experimental/memory_resource.cpp

 }
 
 // default_memory_resource()
 
 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?
         return _VSTD::atomic_exchange_explicit(
             &__res, new_res, memory_order_acq_rel);
     }
     else {
         return _VSTD::atomic_load_explicit(

libcxx/src/ios.cpp

 ios_base::getloc() const
 {
     const locale& loc_storage = *reinterpret_cast<const locale*>(&__loc_);
     return loc_storage;
 }
 
 // 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
 
 template <typename _Tp>
 static size_t __ios_new_cap(size_t __req_size, size_t __current_cap)
 { // Precondition: __req_size > __current_cap
   const size_t mx = std::numeric_limits<size_t>::max() / sizeof(_Tp);

libcxx/test/libcxx/atomics/atomics.flag/init_bool.pass.cpp

 
 #include "test_macros.h"
 
 #if TEST_STD_VER >= 11
 // Ensure that static initialization happens; this is PR#37226
 extern std::atomic_flag global;
 struct X { X() { global.test_and_set(); }};
 X x;
-std::atomic_flag global = ATOMIC_FLAG_INIT;
+std::atomic_flag global{false};
 #endif
 
 int main(int, char**)
 {
 #if TEST_STD_VER >= 11
     assert(global.test_and_set() == 1);
 #endif
     {

libcxx/test/std/atomics/atomics.flag/init.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // XFAIL: c++03
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
 
 // <atomic>
 
 // struct atomic_flag
 
 // atomic_flag() = ATOMIC_FLAG_INIT;
 
 #include <atomic>

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_var_init.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // XFAIL: c++03
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
 
 // <atomic>
 
 // #define ATOMIC_VAR_INIT(value)
 
 #include <atomic>
 #include <type_traits>
 #include <cassert>

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/ctor.pass.cpp

void operator()() const {
     {
       constexpr Atomic a(t);
       assert(a == t);
     }
     {
       constexpr Atomic a{t};
       assert(a == t);
     }
-    {
-      constexpr Atomic a = ATOMIC_VAR_INIT(t);
-      assert(a == t);
-    }
   }
 };
 
 int main(int, char**) {
   TestFunc<UserType>()();
   TestEachIntegralType<TestFunc>()();
 
   return 0;
 }

libcxx/test/std/thread/futures/futures.async/async.pass.cpp

 #include <memory>
 #include <cassert>
 
 #include "test_macros.h"
 
 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()
 {
     invoked = true;
     std::this_thread::sleep_for(ms(200));
     return 3;
 }