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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/ccomplex:24
+#if !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS) && _LIBCPP_STD_VER >= 17
+#  warning `<ccomplex>` is deprecated in C++17 and removed in C++20
+#endif
----------------
Otherwise it just results in problems. Clang should just remove the quotes if the warning consists only of a string literal.


================
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:
> * "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).


================
Comment at: libcxx/include/complex.h:27
 #ifdef __cplusplus
-#  include <ccomplex>
+#  include <__assert>
+#  include <complex>
----------------
Why do we need this include now?


================
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
----------------
Where did you get c++0x and c++1y from? I can't find anything in the code base.


================
Comment at: libcxx/test/libcxx/transitive_includes.sh.cpp:52
 # for std in c++03 c++11 c++14 c++17 c++20 c++2b; do <build>/bin/llvm-lit --param std=$std ${path_to_this_file}; done
-regenerate_expected_results = False
+regenerate_expected_results = True
 # Used because the sequence of tokens RUN : can't appear anywhere or it'll confuse Lit.
----------------



================
Comment at: libcxx/utils/generate_header_tests.py:64-67
+
+    "ccomplex": "__cplusplus < 201703L",
+    "cstdbool": "__cplusplus < 201703L",
+    "ctgmath": "__cplusplus < 201703L",
----------------
We shouldn't disable the tests just because the headers are deprecated. It's still fine to use them in C++17.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146675