aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.
================
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.
----------------
Quuxplusone wrote:
> 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.
The point I was trying to make is that including `<stdatomic.h>` in C++20 and
later mode will diagnose these as deprecated, independent of `<atomic>`. I did
this on the assumption that they're just as useless coming from `<stdatomic.h>`
as they are coming from `<atomic>`, so they should also be diagnosed. I could
make this more clear by mentioning `<atomic>` explicitly in the release note.
As for whether this should be listed under the C++ section... meh. I'm not
certain it matters that much as this is about the C header file. I can mention
it in the C++ section though.
================
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
----------------
Quuxplusone wrote:
> 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.
Thanks for the feedback, I've adjusted the text.
================
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:
> 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.
> :)
> 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.
Yes, it is novel. This is our first foray down this path.
> 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. :)
Not quite. It still matters for a C++ TU that includes `<stdatomic.h>` directly
rather than `<atomic>`, which is still possible.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112221/new/
https://reviews.llvm.org/D112221
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
