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 @@ -24,6 +24,7 @@ #include "Headers.h" #include "ParsedAST.h" #include "clang/Basic/SourceLocation.h" +#include "clang/Lex/HeaderSearch.h" #include "llvm/ADT/DenseSet.h" #include @@ -55,10 +56,12 @@ const SourceManager &SM); /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs). -/// FileIDs that are not true files ( etc) are dropped. +/// FileIDs that are not true files ( etc) and headers without +/// include guards are dropped. llvm::DenseSet translateToHeaderIDs(const llvm::DenseSet &Files, - const IncludeStructure &Includes, const SourceManager &SM); + const IncludeStructure &Includes, const SourceManager &SM, + HeaderSearch &HeaderSearch); /// Retrieves headers that are referenced from the main file but not used. std::vector 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 @@ -14,6 +14,8 @@ #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" @@ -251,8 +253,8 @@ llvm::DenseSet translateToHeaderIDs(const llvm::DenseSet &Files, - const IncludeStructure &Includes, - const SourceManager &SM) { + const IncludeStructure &Includes, const SourceManager &SM, + HeaderSearch &HeaderInfo) { llvm::DenseSet TranslatedHeaderIDs; TranslatedHeaderIDs.reserve(Files.size()); for (FileID FID : Files) { @@ -261,6 +263,13 @@ assert(isSpecialBuffer(FID, SM)); continue; } + // Headers without include guards have side effects and are not + // self-contained, skip them. + if (!HeaderInfo.isFileMultipleIncludeGuarded(FE)) { + dlog("{0} doesn't have header guard and will not be considered unused", + FE->getName()); + continue; + } const auto File = Includes.getID(FE); assert(File); TranslatedHeaderIDs.insert(*File); @@ -272,8 +281,9 @@ const auto &SM = AST.getSourceManager(); auto Refs = findReferencedLocations(AST); - auto ReferencedFiles = translateToHeaderIDs(findReferencedFiles(Refs, SM), - AST.getIncludeStructure(), SM); + auto ReferencedFiles = translateToHeaderIDs( + findReferencedFiles(Refs, SM), AST.getIncludeStructure(), SM, + AST.getPreprocessor().getHeaderSearchInfo()); return getUnused(AST.getIncludeStructure(), ReferencedFiles); } 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 @@ -1494,9 +1494,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 @@ -9,6 +9,7 @@ #include "Annotations.h" #include "IncludeCleaner.h" #include "TestTU.h" +#include "llvm/ADT/StringRef.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -16,6 +17,10 @@ namespace clangd { namespace { +std::string guard(llvm::StringRef Code) { + return "#pragma once\n" + Code.str(); +} + using ::testing::ElementsAre; using ::testing::UnorderedElementsAre; @@ -216,13 +221,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(); @@ -252,6 +257,9 @@ // ScratchBuffer with `flags::FLAGS_FOO` that will have FileID but not // FileEntry. TU.AdditionalFiles["macros.h"] = R"cpp( + #ifndef MACROS_H + #define MACROS_H + #define DEFINE_FLAG(X) \ namespace flags { \ int FLAGS_##X; \ @@ -261,6 +269,8 @@ #define ab x #define concat(x, y) x##y + + #endif // MACROS_H )cpp"; TU.ExtraArgs = {"-DCLI=42"}; ParsedAST AST = TU.build(); @@ -278,7 +288,9 @@ testPath("macros.h"))); // Should not crash due to FileIDs that are not headers. - auto ReferencedHeaders = translateToHeaderIDs(ReferencedFiles, Includes, SM); + auto ReferencedHeaders = + translateToHeaderIDs(ReferencedFiles, Includes, SM, + AST.getPreprocessor().getHeaderSearchInfo()); std::vector ReferencedHeaderNames; for (IncludeStructure::HeaderID HID : ReferencedHeaders) ReferencedHeaderNames.push_back(Includes.getRealPath(HID));