[PATCH] D125209: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks
Whisperity via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed May 11 10:34:46 PDT 2022
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
More information about the cfe-commits
mailing list