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@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
