diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h --- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h +++ b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h @@ -18,6 +18,7 @@ #include "clang/Basic/LLVM.h" #include "clang/Basic/SourceLocation.h" #include "clang/Lex/HeaderSearch.h" +#include "clang/Lex/Preprocessor.h" #include "llvm/Support/Regex.h" #include @@ -42,7 +43,7 @@ private: include_cleaner::RecordedPP RecordedPreprocessor; include_cleaner::PragmaIncludes RecordedPI; - HeaderSearch *HS; + const Preprocessor *PP = nullptr; std::vector IgnoreHeaders; // Whether emit only one finding per usage of a symbol. const bool DeduplicateFindings; diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp --- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp @@ -87,7 +87,7 @@ Preprocessor *PP, Preprocessor *ModuleExpanderPP) { PP->addPPCallbacks(RecordedPreprocessor.record(*PP)); - HS = &PP->getHeaderSearchInfo(); + this->PP = PP; RecordedPI.record(*PP); } @@ -121,7 +121,7 @@ // FIXME: Find a way to have less code duplication between include-cleaner // analysis implementation and the below code. walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI, - *SM, + *PP, [&](const include_cleaner::SymbolReference &Ref, llvm::ArrayRef Providers) { // Process each symbol once to reduce noise in the findings. @@ -200,8 +200,8 @@ tooling::HeaderIncludes HeaderIncludes(getCurrentMainFile(), Code, FileStyle->IncludeStyle); for (const auto &Inc : Missing) { - std::string Spelling = - include_cleaner::spellHeader({Inc.Missing, *HS, MainFile}); + std::string Spelling = include_cleaner::spellHeader( + {Inc.Missing, PP->getHeaderSearchInfo(), MainFile}); bool Angled = llvm::StringRef{Spelling}.starts_with("<"); // We might suggest insertion of an existing include in edge cases, e.g., // include is present in a PP-disabled region, or spelling of the header diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -1245,12 +1245,11 @@ } void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI, const Inclusion &Inc) { - const SourceManager &SM = AST.getSourceManager(); auto Converted = convertIncludes(AST); llvm::DenseSet UsedSymbols; include_cleaner::walkUsed( AST.getLocalTopLevelDecls(), collectMacroReferences(AST), - AST.getPragmaIncludes().get(), SM, + AST.getPragmaIncludes().get(), AST.getPreprocessor(), [&](const include_cleaner::SymbolReference &Ref, llvm::ArrayRef Providers) { if (Ref.RT != include_cleaner::RefType::Explicit || diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -394,7 +394,7 @@ trace::Span Tracer("include_cleaner::walkUsed"); include_cleaner::walkUsed( AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros, - AST.getPragmaIncludes().get(), SM, + AST.getPragmaIncludes().get(), AST.getPreprocessor(), [&](const include_cleaner::SymbolReference &Ref, llvm::ArrayRef Providers) { bool Satisfied = false; diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -1339,7 +1339,7 @@ auto Converted = convertIncludes(AST); include_cleaner::walkUsed( AST.getLocalTopLevelDecls(), collectMacroReferences(AST), - AST.getPragmaIncludes().get(), SM, + AST.getPragmaIncludes().get(), AST.getPreprocessor(), [&](const include_cleaner::SymbolReference &Ref, llvm::ArrayRef Providers) { if (Ref.RT != include_cleaner::RefType::Explicit || diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h --- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h +++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h @@ -15,6 +15,7 @@ #include "clang-include-cleaner/Types.h" #include "clang/Format/Format.h" #include "clang/Lex/HeaderSearch.h" +#include "clang/Lex/Preprocessor.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" @@ -56,7 +57,8 @@ /// the headers for any referenced symbol void walkUsed(llvm::ArrayRef ASTRoots, llvm::ArrayRef MacroRefs, - const PragmaIncludes *PI, const SourceManager &, UsedSymbolCB CB); + const PragmaIncludes *PI, const Preprocessor &PP, + UsedSymbolCB CB); struct AnalysisResults { std::vector Unused; @@ -72,8 +74,7 @@ AnalysisResults analyze(llvm::ArrayRef ASTRoots, llvm::ArrayRef MacroRefs, const Includes &I, - const PragmaIncludes *PI, const SourceManager &SM, - const HeaderSearch &HS, + const PragmaIncludes *PI, const Preprocessor &PP, llvm::function_ref HeaderFilter = nullptr); /// Removes unused includes and inserts missing ones in the main file. diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -17,6 +17,7 @@ #include "clang/Basic/SourceManager.h" #include "clang/Format/Format.h" #include "clang/Lex/HeaderSearch.h" +#include "clang/Lex/Preprocessor.h" #include "clang/Tooling/Core/Replacement.h" #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/ArrayRef.h" @@ -28,14 +29,30 @@ #include "llvm/ADT/StringSet.h" #include "llvm/Support/Error.h" #include "llvm/Support/ErrorHandling.h" +#include +#include #include namespace clang::include_cleaner { +namespace { +bool shouldIgnoreMacroReference(const Preprocessor &PP, const Macro &M) { + auto *MI = PP.getMacroInfo(M.Name); + // Macros that expand to themselves are confusing from user's point of view. + // They usually aspect the usage to be attributed to the underlying decl and + // not the macro definition. So ignore such macros (e.g. std{in,out,err} are + // implementation defined macros, that just resolve to themselves in + // practice). + return MI && MI->getNumTokens() == 1 && MI->isObjectLike() && + MI->getReplacementToken(0).getIdentifierInfo() == M.Name; +} +} // namespace + void walkUsed(llvm::ArrayRef ASTRoots, llvm::ArrayRef MacroRefs, - const PragmaIncludes *PI, const SourceManager &SM, + const PragmaIncludes *PI, const Preprocessor &PP, UsedSymbolCB CB) { + const auto &SM = PP.getSourceManager(); // This is duplicated in writeHTMLReport, changes should be mirrored there. tooling::stdlib::Recognizer Recognizer; for (auto *Root : ASTRoots) { @@ -51,7 +68,8 @@ } for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); - if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation))) + if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)) || + shouldIgnoreMacroReference(PP, MacroRef.Target.macro())) continue; CB(MacroRef, headersForSymbol(MacroRef.Target, SM, PI)); } @@ -60,15 +78,15 @@ AnalysisResults analyze(llvm::ArrayRef ASTRoots, llvm::ArrayRef MacroRefs, const Includes &Inc, - const PragmaIncludes *PI, const SourceManager &SM, - const HeaderSearch &HS, + const PragmaIncludes *PI, const Preprocessor &PP, llvm::function_ref HeaderFilter) { + auto &SM = PP.getSourceManager(); const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID()); llvm::DenseSet Used; llvm::StringSet<> Missing; if (!HeaderFilter) HeaderFilter = [](llvm::StringRef) { return false; }; - walkUsed(ASTRoots, MacroRefs, PI, SM, + walkUsed(ASTRoots, MacroRefs, PI, PP, [&](const SymbolReference &Ref, llvm::ArrayRef
Providers) { bool Satisfied = false; for (const Header &H : Providers) { @@ -82,7 +100,8 @@ if (!Satisfied && !Providers.empty() && Ref.RT == RefType::Explicit && !HeaderFilter(Providers.front().resolvedPath())) - Missing.insert(spellHeader({Providers.front(), HS, MainFile})); + Missing.insert(spellHeader( + {Providers.front(), PP.getHeaderSearchInfo(), MainFile})); }); AnalysisResults Results; diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp --- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp +++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp @@ -153,14 +153,14 @@ if (!HTMLReportPath.empty()) writeHTML(); - auto &HS = getCompilerInstance().getPreprocessor().getHeaderSearchInfo(); llvm::StringRef Path = SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName(); assert(!Path.empty() && "Main file path not known?"); llvm::StringRef Code = SM.getBufferData(SM.getMainFileID()); - auto Results = analyze(AST.Roots, PP.MacroReferences, PP.Includes, &PI, SM, - HS, HeaderFilter); + auto Results = + analyze(AST.Roots, PP.MacroReferences, PP.Includes, &PI, + getCompilerInstance().getPreprocessor(), HeaderFilter); if (!Insert) Results.Missing.clear(); if (!Remove) diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp --- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -66,8 +66,9 @@ } std::multimap> - offsetToProviders(TestAST &AST, SourceManager &SM, + offsetToProviders(TestAST &AST, llvm::ArrayRef MacroRefs = {}) { + const auto &SM = AST.sourceManager(); llvm::SmallVector TopLevelDecls; for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation()))) @@ -75,7 +76,7 @@ TopLevelDecls.emplace_back(D); } std::multimap> OffsetToProviders; - walkUsed(TopLevelDecls, MacroRefs, &PI, SM, + walkUsed(TopLevelDecls, MacroRefs, &PI, AST.preprocessor(), [&](const SymbolReference &Ref, llvm::ArrayRef
Providers) { auto [FID, Offset] = SM.getDecomposedLoc(Ref.RefLocation); if (FID != SM.getMainFileID()) @@ -118,7 +119,7 @@ auto VectorSTL = Header(*tooling::stdlib::Header::named("")); auto UtilitySTL = Header(*tooling::stdlib::Header::named("")); EXPECT_THAT( - offsetToProviders(AST, SM), + offsetToProviders(AST), UnorderedElementsAre( Pair(Code.point("bar"), UnorderedElementsAre(MainFile)), Pair(Code.point("private"), @@ -155,7 +156,7 @@ auto HeaderFile2 = Header(AST.fileManager().getFile("header2.h").get()); auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID())); EXPECT_THAT( - offsetToProviders(AST, SM), + offsetToProviders(AST), Contains(Pair(Code.point("foo"), UnorderedElementsAre(HeaderFile1, HeaderFile2, MainFile)))); } @@ -171,19 +172,19 @@ Inputs.ExtraFiles["hdr.h"] = Hdr.code(); TestAST AST(Inputs); auto &SM = AST.sourceManager(); + auto &PP = AST.preprocessor(); const auto *HdrFile = SM.getFileManager().getFile("hdr.h").get(); auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID())); auto HdrID = SM.translateFile(HdrFile); - IdentifierTable Idents; - Symbol Answer1 = - Macro{&Idents.get("ANSWER"), SM.getComposedLoc(HdrID, Hdr.point())}; - Symbol Answer2 = - Macro{&Idents.get("ANSWER"), SM.getComposedLoc(HdrID, Hdr.point())}; + Symbol Answer1 = Macro{PP.getIdentifierInfo("ANSWER"), + SM.getComposedLoc(HdrID, Hdr.point())}; + Symbol Answer2 = Macro{PP.getIdentifierInfo("ANSWER"), + SM.getComposedLoc(HdrID, Hdr.point())}; EXPECT_THAT( offsetToProviders( - AST, SM, + AST, {SymbolReference{ Answer1, SM.getComposedLoc(SM.getMainFileID(), Code.point("1")), RefType::Explicit}, @@ -240,8 +241,7 @@ auto Decls = AST.context().getTranslationUnitDecl()->decls(); auto Results = analyze(std::vector{Decls.begin(), Decls.end()}, - PP.MacroReferences, PP.Includes, &PI, AST.sourceManager(), - AST.preprocessor().getHeaderSearchInfo()); + PP.MacroReferences, PP.Includes, &PI, AST.preprocessor()); const Include *B = PP.Includes.atLine(3); ASSERT_EQ(B->Spelled, "b.h"); @@ -257,8 +257,7 @@ Inputs.FileName = "public.h"; TestAST AST(Inputs); EXPECT_FALSE(PP.Includes.all().empty()); - auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(), - AST.preprocessor().getHeaderSearchInfo()); + auto Results = analyze({}, {}, PP.Includes, &PI, AST.preprocessor()); EXPECT_THAT(Results.Unused, testing::IsEmpty()); } @@ -268,8 +267,7 @@ Inputs.ErrorOK = true; TestAST AST(Inputs); EXPECT_FALSE(PP.Includes.all().empty()); - auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(), - AST.preprocessor().getHeaderSearchInfo()); + auto Results = analyze({}, {}, PP.Includes, &PI, AST.preprocessor()); EXPECT_THAT(Results.Unused, testing::IsEmpty()); } @@ -409,7 +407,7 @@ SourceLocation RefLoc; walkUsed(TopLevelDecls, Recorded.MacroReferences, - /*PragmaIncludes=*/nullptr, SM, + /*PragmaIncludes=*/nullptr, AST.preprocessor(), [&](const SymbolReference &Ref, llvm::ArrayRef
) { if (!Ref.RefLocation.isMacroID()) return; @@ -472,7 +470,7 @@ const auto *Partial = SM.getFileManager().getFile("partial.h").get(); EXPECT_THAT( - offsetToProviders(AST, SM), + offsetToProviders(AST), AllOf(Contains( Pair(Code.point("exp_spec"), UnorderedElementsAre(Fwd, Def))), Contains(Pair(Code.point("exp"), UnorderedElementsAre(Fwd, Def))), @@ -483,5 +481,29 @@ Pair(Code.point("partial"), UnorderedElementsAre(Partial))))); } +TEST_F(WalkUsedTest, IgnoresIdentityMacros) { + llvm::Annotations Code(R"cpp( + #include "header.h" + void $bar^bar() { + $stdin^stdin(); + } + )cpp"); + Inputs.Code = Code.code(); + Inputs.ExtraFiles["header.h"] = guard(R"cpp( + #include "inner.h" + void stdin(); + )cpp"); + Inputs.ExtraFiles["inner.h"] = guard(R"cpp( + #define stdin stdin + )cpp"); + + TestAST AST(Inputs); + auto &SM = AST.sourceManager(); + auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID())); + EXPECT_THAT(offsetToProviders(AST), + UnorderedElementsAre( + // FIXME: we should have a reference from stdin to header.h + Pair(Code.point("bar"), UnorderedElementsAre(MainFile)))); +} } // namespace } // namespace clang::include_cleaner