[PATCH] D112221: Mark ATOMIC_VAR_INIT and ATOMIC_FLAG_INIT as deprecated

Wed Dec 15 09:49:03 PST 2021

aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

In D112221#3119567 <https://reviews.llvm.org/D112221#3119567>, @Quuxplusone wrote:

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

Thank you for the help here! FWIW, I'm not certain the libc++ tests need to be changed at all -- shouldn't those all be using the macro definitions obtained by libc++'s `<atomic>` header? If so, those are not flagged as deprecated yet: https://github.com/llvm/llvm-project/blob/main/libcxx/include/atomic#L2699

================
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
----------------
Quuxplusone wrote:
> 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.
I think this is a reasonable idea. Microsoft has macros for a similar purpose with `_CRT_SECURE_NO_WARNINGS` (IIRC). I would not want to use `_LIBCPP_`  for this purpose, but I could imagine it's useful to add something like `_CLANG_DISABLE_DEPRECATION_WARNINGS`. (I'm guessing we don't want per-deprecation granularity on the macro, but if we needed something like that, I think we could add it.)

I went ahead and added `_CLANG_DISABLE_CRT_DEPRECATION_WARNINGS` to the next version of the patch, and documented it in the release notes and user's manual. We can quibble over the name if you have better suggestions.

================
Comment at: clang/test/Headers/stdatomic-deprecations.c:11
+void func(void) {
+  (void)ATOMIC_VAR_INIT(12); // expected-warning {{macro 'ATOMIC_VAR_INIT' has been marked as deprecated}} \
+                             // expected-note at stdatomic.h:* {{macro marked 'deprecated' here}}
----------------
Quuxplusone wrote:
> This doesn't look like correct use of the `ATOMIC_VAR_INIT` macro. It should be initializing an atomic, yeah?
> (Note for example that if you did `(void)ATOMIC_FLAG_INIT(12)`, even with libc++'s implementation, it would just fail with a syntax error, because `(void){12}` is not a valid expression AFAIK.)
> This doesn't look like correct use of the ATOMIC_VAR_INIT macro. It should be initializing an atomic, yeah?

It expands to `(void)(12);` which is why the test works as it stands today and is valid. However, I don't mind changing it to use an actual init instead.

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

https://reviews.llvm.org/D112221