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 @@ -55,7 +55,8 @@ 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); 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" @@ -186,12 +188,27 @@ } } -// 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. -bool mayConsiderUnused(const Inclusion *Inc) { - return Inc->Written.front() != '<'; +bool mayConsiderUnused(const Inclusion *Inc, const IncludeStructure &Includes, + FileManager &FM, HeaderSearch &HeaderInfo) { + // 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. + if (Inc->Written.front() == '<') + return false; + // Headers without include guards have side effects and are not + // self-contained, skip them. + assert(Inc->HeaderID); + auto FE = FM.getFile(Includes.getRealPath( + static_cast(*Inc->HeaderID))); + assert(FE); + if (!HeaderInfo.isFileMultipleIncludeGuarded(*FE)) { + dlog("{0} doesn't have header guard and will not be considered unused", + (*FE)->getName()); + return false; + } + return true; } } // namespace @@ -291,7 +308,9 @@ ->getName() .str(); for (const auto *Inc : computeUnusedIncludes(AST)) { - if (!mayConsiderUnused(Inc)) + if (!mayConsiderUnused(Inc, AST.getIncludeStructure(), + AST.getSourceManager().getFileManager(), + AST.getPreprocessor().getHeaderSearchInfo())) continue; Diag D; D.Message = 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,9 @@ TEST(DiagnosticsTest, IncludeCleaner) { Annotations Test(R"cpp( $fix[[ $diag[[#include "unused.h"]] -]]#include "used.h" +]] + #include "no_guard.h" + #include "used.h" #include @@ -1494,9 +1496,16 @@ TestTU TU; TU.Code = Test.code().str(); TU.AdditionalFiles["unused.h"] = R"cpp( + #pragma once void unused() {} )cpp"; + // This header is unused but doesn't have a header guard meaning it is not + // self-contained and can have side effects. There will be no warning for it. + TU.AdditionalFiles["no_guard.h"] = R"cpp( + int side_effects = 42; + )cpp"; TU.AdditionalFiles["used.h"] = R"cpp( + #pragma once void used() {} )cpp"; TU.AdditionalFiles["system/system_header.h"] = "";