[PATCH] D125209: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks

[Balázs Benics via Phabricator via cfe-commits]

steakhal marked 2 inline comments as done.
steakhal added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:70-73
+    auto ExternCBlockBegin =
+        SM.getDecomposedExpansionLoc(LinkSpecDecl->getBeginLoc());
+    auto ExternCBlockEnd =
+        SM.getDecomposedExpansionLoc(LinkSpecDecl->getEndLoc());
----------------
whisperity wrote:
> This unconditionally assumes that an encountered LSD will be `extern "C"`. However, `extern "C++"` is also possible (and supported by Clang). (And things like `extern "Java"` are also possible according to the standard, albeit not supported by Clang.)
O.O I did not know its a thing :D
It seems like clang only supports `C` or `C++`.


================
Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:87-88
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
-    PP->addPPCallbacks(
-        ::std::make_unique<IncludeModernizePPCallbacks>(*this, getLangOpts()));
+  PP->addPPCallbacks(::std::make_unique<detail::IncludeModernizePPCallbacks>(
+      *this, getLangOpts(), PP->getSourceManager()));
+}
----------------
whisperity wrote:
> (🔮: Same here. Is this state that might keep a dangling reference if multiple TUs are consumed in sequence?)
How can I acquire the right `SourceManager` if not like this?
I haven't found any alternative.
What do you suggest?


================
Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:135
             {"float.h", "cfloat"},
             {"limits.h", "climits"},
             {"locale.h", "clocale"},
----------------
whisperity wrote:
> POSIX defines a "global" include `limits.h` which gives you the definition of macros like `PATH_MAX`. Will such code keep on working if the include is turned into `climits`?
IDK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125209