VitaNuo updated this revision to Diff 549443. VitaNuo marked 4 inline comments as done. VitaNuo added a comment.
Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157610/new/ https://reviews.llvm.org/D157610 Files: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp clang-tools-extra/clangd/IncludeCleaner.cpp clang-tools-extra/clangd/test/include-cleaner-batch-fix.test clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp clang-tools-extra/include-cleaner/lib/Analysis.cpp clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
Index: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp =================================================================== --- clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -273,6 +273,30 @@ EXPECT_THAT(Results.Unused, testing::IsEmpty()); } +TEST_F(AnalyzeTest, ResourceDirIsIgnored) { + Inputs.ExtraArgs.push_back("-resource-dir"); + Inputs.ExtraArgs.push_back("./resources"); + Inputs.Code = R"cpp( + #include "resources/amintrin.h" + #include "resources/imintrin.h" + void baz() { + bar(); + } + )cpp"; + Inputs.ExtraFiles["resources/amintrin.h"] = ""; + Inputs.ExtraFiles["resources/emintrin.h"] = guard(R"cpp( + void bar(); + )cpp"); + Inputs.ExtraFiles["resources/imintrin.h"] = guard(R"cpp( + #include "emintrin.h" + )cpp"); + TestAST AST(Inputs); + auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(), + AST.preprocessor().getHeaderSearchInfo()); + EXPECT_THAT(Results.Unused, testing::IsEmpty()); + EXPECT_THAT(Results.Missing, testing::IsEmpty()); +} + TEST(FixIncludes, Basic) { llvm::StringRef Code = R"cpp(#include "d.h" #include "a.h" Index: clang-tools-extra/include-cleaner/lib/Analysis.cpp =================================================================== --- clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ 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/HeaderSearchOptions.h" #include "clang/Tooling/Core/Replacement.h" #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/ArrayRef.h" @@ -68,12 +69,18 @@ llvm::StringSet<> Missing; if (!HeaderFilter) HeaderFilter = [](llvm::StringRef) { return false; }; + std::string ResourceDir = HS.getHeaderSearchOpts().ResourceDir; walkUsed(ASTRoots, MacroRefs, PI, SM, [&](const SymbolReference &Ref, llvm::ArrayRef<Header> Providers) { bool Satisfied = false; for (const Header &H : Providers) { - if (H.kind() == Header::Physical && H.physical() == MainFile) + if (H.kind() == Header::Physical && + (H.physical() == MainFile || + H.physical()->getLastRef().getDir().getName() == + ResourceDir)) { Satisfied = true; + continue; + } for (const Include *I : Inc.match(H)) { Used.insert(I); Satisfied = true; @@ -88,7 +95,8 @@ AnalysisResults Results; for (const Include &I : Inc.all()) { if (Used.contains(&I) || !I.Resolved || - HeaderFilter(I.Resolved->getFileEntry().tryGetRealPathName())) + HeaderFilter(I.Resolved->getFileEntry().tryGetRealPathName()) || + I.Resolved->getDir().getName() == ResourceDir) continue; if (PI) { if (PI->shouldKeep(*I.Resolved)) Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -574,6 +574,29 @@ EXPECT_THAT(Findings.UnusedIncludes, IsEmpty()); } +TEST(IncludeCleaner, ResourceDirIsIgnored) { + auto TU = TestTU::withCode(R"cpp( + #include <amintrin.h> + #include <imintrin.h> + void baz() { + bar(); + } + )cpp"); + TU.ExtraArgs.push_back("-resource-dir"); + TU.ExtraArgs.push_back(testPath("resources")); + TU.AdditionalFiles["resources/include/amintrin.h"] = ""; + TU.AdditionalFiles["resources/include/imintrin.h"] = guard(R"cpp( + #include <emintrin.h> + )cpp"); + TU.AdditionalFiles["resources/include/emintrin.h"] = guard(R"cpp( + void bar(); + )cpp"); + auto AST = TU.build(); + auto Findings = computeIncludeCleanerFindings(AST); + EXPECT_THAT(Findings.UnusedIncludes, IsEmpty()); + EXPECT_THAT(Findings.MissingIncludes, IsEmpty()); +} + } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/test/include-cleaner-batch-fix.test =================================================================== --- clang-tools-extra/clangd/test/include-cleaner-batch-fix.test +++ clang-tools-extra/clangd/test/include-cleaner-batch-fix.test @@ -5,10 +5,11 @@ # RUN: rm -rf %t # RUN: mkdir -p %t/clangd # RUN: cp -r %S/Inputs/include-cleaner %t/include +# RUN: echo '-I%t/include' > %t/compile_flags.txt # Create a config file enabling include-cleaner features. # RUN: echo $'Diagnostics:\n UnusedIncludes: Strict\n MissingIncludes: Strict' >> %t/clangd/config.yaml -# RUN: env XDG_CONFIG_HOME=%t clangd -lit-test -enable-config --resource-dir=%t < %s | FileCheck -strict-whitespace %s +# RUN: env XDG_CONFIG_HOME=%t clangd -lit-test -enable-config --compile-commands-dir=%t < %s | FileCheck -strict-whitespace %s {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"workspaceEdit":{"documentChanges":true, "changeAnnotationSupport":{"groupsOnLabel":true}}}},"trace":"off"}} --- { Index: clang-tools-extra/clangd/IncludeCleaner.cpp =================================================================== --- clang-tools-extra/clangd/IncludeCleaner.cpp +++ clang-tools-extra/clangd/IncludeCleaner.cpp @@ -75,6 +75,11 @@ auto FE = AST.getSourceManager().getFileManager().getFileRef( AST.getIncludeStructure().getRealPath(HID)); assert(FE); + if (FE->getDir() == AST.getPreprocessor() + .getHeaderSearchInfo() + .getModuleMap() + .getBuiltinDir()) + return false; if (PI && PI->shouldKeep(*FE)) return false; // FIXME(kirillbobyrev): We currently do not support the umbrella headers. @@ -392,6 +397,10 @@ std::vector<MissingIncludeDiagInfo> MissingIncludes; llvm::DenseSet<IncludeStructure::HeaderID> Used; trace::Span Tracer("include_cleaner::walkUsed"); + const DirectoryEntry *ResourceDir = AST.getPreprocessor() + .getHeaderSearchInfo() + .getModuleMap() + .getBuiltinDir(); include_cleaner::walkUsed( AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros, AST.getPragmaIncludes().get(), SM, @@ -400,7 +409,8 @@ bool Satisfied = false; for (const auto &H : Providers) { if (H.kind() == include_cleaner::Header::Physical && - (H.physical() == MainFile || H.physical() == PreamblePatch)) { + (H.physical() == MainFile || H.physical() == PreamblePatch || + H.physical()->getLastRef().getDir() == ResourceDir)) { Satisfied = true; continue; } Index: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp +++ clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp @@ -26,6 +26,7 @@ #include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" #include "clang/Format/Format.h" +#include "clang/Lex/HeaderSearchOptions.h" #include "clang/Lex/Preprocessor.h" #include "clang/Tooling/Core/Replacement.h" #include "clang/Tooling/Inclusions/HeaderIncludes.h" @@ -118,47 +119,52 @@ MainFileDecls.push_back(D); } llvm::DenseSet<include_cleaner::Symbol> SeenSymbols; + std::string ResourceDir = HS->getHeaderSearchOpts().ResourceDir; // FIXME: Find a way to have less code duplication between include-cleaner // analysis implementation and the below code. - walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI, - *SM, - [&](const include_cleaner::SymbolReference &Ref, - llvm::ArrayRef<include_cleaner::Header> Providers) { - // Process each symbol once to reduce noise in the findings. - // Tidy checks are used in two different workflows: - // - Ones that show all the findings for a given file. For such - // workflows there is not much point in showing all the occurences, - // as one is enough to indicate the issue. - // - Ones that show only the findings on changed pieces. For such - // workflows it's useful to show findings on every reference of a - // symbol as otherwise tools might give incosistent results - // depending on the parts of the file being edited. But it should - // still help surface findings for "new violations" (i.e. - // dependency did not exist in the code at all before). - if (DeduplicateFindings && !SeenSymbols.insert(Ref.Target).second) - return; - bool Satisfied = false; - for (const include_cleaner::Header &H : Providers) { - if (H.kind() == include_cleaner::Header::Physical && - H.physical() == MainFile) - Satisfied = true; - - for (const include_cleaner::Include *I : - RecordedPreprocessor.Includes.match(H)) { - Used.insert(I); - Satisfied = true; - } - } - if (!Satisfied && !Providers.empty() && - Ref.RT == include_cleaner::RefType::Explicit && - !shouldIgnore(Providers.front())) - Missing.push_back({Ref, Providers.front()}); - }); + walkUsed( + MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI, *SM, + [&](const include_cleaner::SymbolReference &Ref, + llvm::ArrayRef<include_cleaner::Header> Providers) { + // Process each symbol once to reduce noise in the findings. + // Tidy checks are used in two different workflows: + // - Ones that show all the findings for a given file. For such + // workflows there is not much point in showing all the occurences, + // as one is enough to indicate the issue. + // - Ones that show only the findings on changed pieces. For such + // workflows it's useful to show findings on every reference of a + // symbol as otherwise tools might give incosistent results + // depending on the parts of the file being edited. But it should + // still help surface findings for "new violations" (i.e. + // dependency did not exist in the code at all before). + if (DeduplicateFindings && !SeenSymbols.insert(Ref.Target).second) + return; + bool Satisfied = false; + for (const include_cleaner::Header &H : Providers) { + if (H.kind() == include_cleaner::Header::Physical && + (H.physical() == MainFile || + H.physical()->getLastRef().getDir().getName() == ResourceDir)) { + Satisfied = true; + continue; + } + + for (const include_cleaner::Include *I : + RecordedPreprocessor.Includes.match(H)) { + Used.insert(I); + Satisfied = true; + } + } + if (!Satisfied && !Providers.empty() && + Ref.RT == include_cleaner::RefType::Explicit && + !shouldIgnore(Providers.front())) + Missing.push_back({Ref, Providers.front()}); + }); std::vector<const include_cleaner::Include *> Unused; for (const include_cleaner::Include &I : RecordedPreprocessor.Includes.all()) { - if (Used.contains(&I) || !I.Resolved) + if (Used.contains(&I) || !I.Resolved || + I.Resolved->getDir().getName() == ResourceDir) continue; if (RecordedPI.shouldKeep(*I.Resolved)) continue;
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits