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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits