[libcxx-commits] [PATCH] D146675: [libc++] Warn on including headers that are deprecated in C++17

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/ccomplex:24
+#if !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS) && _LIBCPP_STD_VER >= 17
+#  warning "Header <ccomplex> is deprecated in C++17"
+#endif
----------------
cjdb wrote:
> philnik wrote:
> > cjdb wrote:
> > > * "removed as of C++20" increases the urgency of the diagnostic.
> > > * "Header" bit removed since that will be implied by context.
> > > * Added backticks to support SARIF diagnostics rendering this in Markdown.
> > > 
> > > Same comment applies below.
> > I don't think we want to say "and removed in C++20" if we don't actually remove the header. We'd have to move these headers into their own directory and teach clang about that, and this seems like quite a bit of work to remove these headers (although it might clean up the code base a bit).
> > I don't think we want to say "and removed in C++20" if we don't actually remove the header. We'd have to move these headers into their own directory and teach clang about that, and this seems like quite a bit of work to remove these headers (although it might clean up the code base a bit).
> 
> It's implementable in library.
> 
> ```
> #if __cplusplus >= 202003L
> #  error `<ccomplex>` removed in C++20.
> #elif __cplusplus == 201703L
> #  warning `<ccomplex>` deprecated in C++17
> #endif
> ```
> 
> > Otherwise it just results in problems. Clang should just remove the quotes if the warning consists only of a string literal.
> 
> What problems are those?
That would break `__has_include(<ccomplex>)` though. What's the point of that feature if you can't actually rely on it?

The compilers sometimes produce random additional warnings: https://godbolt.org/z/fE7x6E11r



================
Comment at: libcxx/include/tgmath.h:27
 #ifdef __cplusplus
-#  include <ctgmath>
+#  include <__assert>
+#  include <complex>
----------------
Also here.


================
Comment at: libcxx/test/libcxx/assertions/headers_declare_verbose_abort.sh.cpp:49
 
+// Prevent <ccomplex>, <cstdbool>, and <ctgmath from generating deprecated warnings for this test.
+#define _LIBCPP_DISABLE_DEPRECATION_WARNINGS
----------------
Also for the comment in the other tests.


================
Comment at: libcxx/test/libcxx/assertions/headers_declare_verbose_abort.sh.cpp:50
+// Prevent <ccomplex>, <cstdbool>, and <ctgmath from generating deprecated warnings for this test.
+#define _LIBCPP_DISABLE_DEPRECATION_WARNINGS
+
----------------
and move it near the `XFAIL` above.


================
Comment at: libcxx/test/libcxx/language.support/support.runtime/version_cstdbool.pass.cpp:11
 
+// REQUIRES: c++11 || c++0x || c++14 || c++1y
+// XFAIL: c++17
----------------
ayzhao wrote:
> philnik wrote:
> > Where did you get c++0x and c++1y from? I can't find anything in the code base.
> These are mentioned in `params.py`: https://github.com/llvm/llvm-project/blob/de939c6cd80c1e88913f1ef12be11aea501eb89b/libcxx/utils/libcxx/test/params.py#L60-L61
You've got it the wrong way around :). The code you linked to treats them as equal compiler flags, i.e. `-std=c++11` is the same as `-std=c++0x` and `c++11` is the lit feature that's set. So you only have to test `c++11 || c++14 || c++17` (and disable the deprecation warning).


================
Comment at: libcxx/test/std/language.support/support.runtime/cstdbool.pass.cpp:11
 
+// REQUIRES: c++11 || c++0x || c++14 || c++1y
+// XFAIL: c++17
----------------
ayzhao wrote:
> Mordante wrote:
> > Why not using the UNSUPPORTED like we usually do?
> My idea was that by using `XFAIL`, we can test that the deprecation warning works (since we build with `-Werror`).
That should instead be done with a `.verify.cpp`. You can also run the test suite without warnings enabled, and having an XFAIL from a warning would break this. The `.verify.cpp` also checks what warnings/errors are generated instead of just XFAILing for any reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146675