diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -61,8 +61,9 @@ const IncludeStructure &Includes, const SourceManager &SM); /// Retrieves headers that are referenced from the main file but not used. +/// Inclusions of headers with no header guard are dropped. std::vector -getUnused(const IncludeStructure &Includes, +getUnused(ParsedAST &AST, const llvm::DenseSet &ReferencedFiles); std::vector computeUnusedIncludes(ParsedAST &AST); 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 @@ -8,12 +8,15 @@ #include "IncludeCleaner.h" #include "Config.h" +#include "ParsedAST.h" #include "Protocol.h" #include "SourceCode.h" #include "support/Logger.h" #include "support/Trace.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/Basic/SourceLocation.h" +#include "clang/Lex/HeaderSearch.h" +#include "clang/Lex/Preprocessor.h" #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" @@ -187,9 +190,10 @@ } // FIXME(kirillbobyrev): We currently do not support the umbrella headers. -// Standard Library headers are typically umbrella headers, and system headers -// are likely to be the Standard Library headers. Until we have a good support -// for umbrella headers and Standard Library headers, don't warn about them. +// Standard Library headers are typically umbrella headers, and system +// headers are likely to be the Standard Library headers. Until we have a +// good support for umbrella headers and Standard Library headers, don't warn +// about them. bool mayConsiderUnused(const Inclusion *Inc) { return Inc->Written.front() != '<'; } @@ -223,18 +227,31 @@ } std::vector -getUnused(const IncludeStructure &Includes, +getUnused(ParsedAST &AST, const llvm::DenseSet &ReferencedFiles) { std::vector Unused; - for (const Inclusion &MFI : Includes.MainFileIncludes) { - // FIXME: Skip includes that are not self-contained. + for (const Inclusion &MFI : AST.getIncludeStructure().MainFileIncludes) { if (!MFI.HeaderID) { elog("File {0} not found.", MFI.Written); continue; } auto IncludeID = static_cast(*MFI.HeaderID); - if (!ReferencedFiles.contains(IncludeID)) + bool Used = ReferencedFiles.contains(IncludeID); + if (!Used) { + // Headers without include guards have side effects and are not + // self-contained, skip them. + auto FE = AST.getSourceManager().getFileManager().getFile( + AST.getIncludeStructure().getRealPath(IncludeID)); + assert(FE); + if (!AST.getPreprocessor() + .getHeaderSearchInfo() + .isFileMultipleIncludeGuarded(*FE)) { + dlog("{0} has no header guard and is filtered out of unused includes", + (*FE)->getName()); + continue; + } Unused.push_back(&MFI); + } dlog("{0} is {1}", MFI.Written, ReferencedFiles.contains(IncludeID) ? "USED" : "UNUSED"); } @@ -272,9 +289,15 @@ const auto &SM = AST.getSourceManager(); auto Refs = findReferencedLocations(AST); - auto ReferencedFiles = translateToHeaderIDs(findReferencedFiles(Refs, SM), - AST.getIncludeStructure(), SM); - return getUnused(AST.getIncludeStructure(), ReferencedFiles); + // FIXME(kirillbobyrev): Attribute the symbols from non self-contained + // headers to their parents while the information about respective + // SourceLocations and FileIDs is not lost. It is not possible to do that + // later when non-guarded headers included multiple times will get same + // HeaderID. + auto ReferencedFileIDs = findReferencedFiles(Refs, SM); + auto ReferencedHeaders = + translateToHeaderIDs(ReferencedFileIDs, AST.getIncludeStructure(), SM); + return getUnused(AST, ReferencedHeaders); } std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -1483,7 +1483,8 @@ TEST(DiagnosticsTest, IncludeCleaner) { Annotations Test(R"cpp( $fix[[ $diag[[#include "unused.h"]] -]]#include "used.h" +]] + #include "used.h" #include @@ -1494,9 +1495,11 @@ TestTU TU; TU.Code = Test.code().str(); TU.AdditionalFiles["unused.h"] = R"cpp( + #pragma once void unused() {} )cpp"; TU.AdditionalFiles["used.h"] = R"cpp( + #pragma once void used() {} )cpp"; TU.AdditionalFiles["system/system_header.h"] = ""; diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -19,6 +19,10 @@ using ::testing::ElementsAre; using ::testing::UnorderedElementsAre; +std::string guard(llvm::StringRef Code) { + return "#pragma once\n" + Code.str(); +} + TEST(IncludeCleaner, ReferencedLocations) { struct TestCase { std::string HeaderCode; @@ -216,13 +220,13 @@ // Build expected ast with symbols coming from headers. TestTU TU; TU.Filename = "foo.cpp"; - TU.AdditionalFiles["foo.h"] = "void foo();"; - TU.AdditionalFiles["a.h"] = "void a();"; - TU.AdditionalFiles["b.h"] = "void b();"; - TU.AdditionalFiles["dir/c.h"] = "void c();"; - TU.AdditionalFiles["unused.h"] = "void unused();"; - TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();"; - TU.AdditionalFiles["system/system_header.h"] = ""; + TU.AdditionalFiles["foo.h"] = guard("void foo();"); + TU.AdditionalFiles["a.h"] = guard("void a();"); + TU.AdditionalFiles["b.h"] = guard("void b();"); + TU.AdditionalFiles["dir/c.h"] = guard("void c();"); + TU.AdditionalFiles["unused.h"] = guard("void unused();"); + TU.AdditionalFiles["dir/unused.h"] = guard("void dirUnused();"); + TU.AdditionalFiles["system/system_header.h"] = guard(""); TU.ExtraArgs.push_back("-I" + testPath("dir")); TU.ExtraArgs.push_back("-isystem" + testPath("system")); TU.Code = MainFile.str(); @@ -286,7 +290,7 @@ EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h"))); // Sanity check. - EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty()); + EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty()); } } // namespace