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

TLDR on `_CLANG_DISABLE_CRT_DEPRECATION_WARNINGS`: I'm neutral. :)



================
Comment at: clang/docs/ReleaseNotes.rst:159-164
+ - The ``ATOMIC_VAR_INIT`` macro from ``<stdatomic.h>`` is now diagnosed as
+   deprecated in both C (C17 and later) and C++ (C++20 and later), and
+   ``ATOMIC_FLAG_INIT`` is now diagnosed as deprecated in C++ (C++20 and later).
+   The diagnostic can be disabled by defining the
+   ``_CLANG_DISABLE_CRT_DEPRECATION_WARNINGS`` macro prior to including the
+   header.
----------------
IIUC, `<stdatomic.h>` is not relevant to C++20; libc++'s `<atomic>` even `#error`s if you include both `<stdatomic.h>` and `<atomic>` in the same TU.
Defining `_CLANG_DISABLE_CRT_DEPRECATION_WARNINGS` won't affect the warnings in C++20; for those you need `_LIBCPP_DISABLE_DEPRECATION_WARNINGS`.
C++20 isn't relevant to this section on "C Language Changes in Clang"; arguably it could be listed under "C++ Language Changes in Clang," except that D115995 didn't change anything about //Clang//. So I think it's fine not to mention D115995 anywhere in this file.


================
Comment at: clang/docs/UsersManual.rst:1128-1130
+  #include <stdint.h> // May include Clang CRT deprecation warnings
+  #define _CLANG_DISABLE_CRT_DEPRECATION_WARNINGS
+  #include <stdatomic.h> // Clang CRT deprecation warnings are disabled
----------------
It is //probably// sketchy to advise people to change the setting of `_CLANG_DISABLE_CRT_DEPRECATION_WARNINGS` over the lifetime of a single TU. If `<stdint.h>` transitively includes `<stdatomic.h>`, this won't work. (Of course that's unlikely in this //specific// case, but in general, IMHO we shouldn't train people to think that this will always work.)
(In fact, `<stdatomic.h>` transitively includes `<stdint.h>`!)
We should train the programmer to think of `_CLANG_DISABLE_CRT_DEPRECATION_WARNINGS` as all-or-nothing: set it in your build system, or set it at the very top of the TU.


================
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
----------------
aaron.ballman wrote:
> 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.
Re `_CLANG_DISABLE_CRT_DEPRECATION_WARNINGS`: This naming scheme looks novel — I don't see any other `_CLANG_{DIS,EN}ABLE.*` macros in `clang/lib/Headers/` — but that might just be because its purpose is also novel. I'm not saying it's a good idea, but I don't object to it nor have any particularly better idea.

Btw, where above I said `-Wno-deprecated`, it turns out that `-Wno-deprecated-pragma` is slightly finer-grained. Also, I'm like 95% sure that C++ is totally covered by D115995, which means whatever you do here is relevant only to C17, which means I have no personal stake in its ergonomics. :)


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

https://reviews.llvm.org/D112221