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

[Whisperity via Phabricator via cfe-commits]

whisperity 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());
----------------
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.)


================
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()));
+}
----------------
(🔮: Same here. Is this state that might keep a dangling reference if multiple TUs are consumed in sequence?)


================
Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:92
+    ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(ast_matchers::translationUnitDecl().bind("TU"), this);
+}
----------------
(Perhaps it is worth an explicit comment here that you do this at first glance unconventional match because the entire check's logic is calculated **only** on the "preprocessor" level.)


================
Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:135
             {"float.h", "cfloat"},
             {"limits.h", "climits"},
             {"locale.h", "clocale"},
----------------
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`?


================
Comment at: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h:66
+  friend class detail::ExternCRefutationVisitor;
+  std::vector<detail::IncludeMarker> IncludesToBeProcessed;
 };
----------------
🔮: You are storing some TU-specific state here (`SourceRange`) that is not cleared. What happens if you run `clang-tidy a.cpp b.cpp`, i.e. two TUs in the same process execution?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125209