kbobyrev updated this revision to Diff 382968. kbobyrev added a comment. Put the check into the right place.
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112695/new/ https://reviews.llvm.org/D112695 Files: clang-tools-extra/clangd/IncludeCleaner.cpp clang-tools-extra/clangd/IncludeCleaner.h clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ 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" @@ -252,6 +253,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 +265,8 @@ #define ab x #define concat(x, y) x##y + + #endif // MACROS_H )cpp"; TU.ExtraArgs = {"-DCLI=42"}; ParsedAST AST = TU.build(); Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ 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 <system_header.h> @@ -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"] = ""; Index: clang-tools-extra/clangd/IncludeCleaner.h =================================================================== --- clang-tools-extra/clangd/IncludeCleaner.h +++ 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 (<built-in> etc) are dropped. +/// FileIDs that are not true files (<built-in> etc) and headers without +/// include guards are dropped. llvm::DenseSet<IncludeStructure::HeaderID> translateToHeaderIDs(const llvm::DenseSet<FileID> &Files, const IncludeStructure &Includes, const SourceManager &SM); Index: clang-tools-extra/clangd/IncludeCleaner.cpp =================================================================== --- clang-tools-extra/clangd/IncludeCleaner.cpp +++ 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<IncludeStructure::HeaderID>(*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 =
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits