kbobyrev updated this revision to Diff 383250.
kbobyrev marked 3 inline comments as done.
kbobyrev added a comment.
Address review comments.
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
@@ -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;
@@ -206,6 +210,7 @@
#include "b.h"
#include "dir/c.h"
#include "dir/unused.h"
+ #include "unguarded.h"
#include "unused.h"
#include <system_header.h>
void foo() {
@@ -216,13 +221,14 @@
// 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.AdditionalFiles["unguarded.h"] = "";
TU.ExtraArgs.push_back("-I" + testPath("dir"));
TU.ExtraArgs.push_back("-isystem" + testPath("system"));
TU.Code = MainFile.str();
@@ -231,8 +237,7 @@
for (const auto &Include : computeUnusedIncludes(AST))
UnusedIncludes.push_back(Include->Written);
EXPECT_THAT(UnusedIncludes,
- UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\"",
- "<system_header.h>"));
+ UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
}
TEST(IncludeCleaner, VirtualBuffers) {
@@ -286,7 +291,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
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,8 @@
TEST(DiagnosticsTest, IncludeCleaner) {
Annotations Test(R"cpp(
$fix[[ $diag[[#include "unused.h"]]
-]]#include "used.h"
+]]
+ #include "used.h"
#include <system_header.h>
@@ -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"] = "";
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ 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.
+/// System headers and inclusions of headers with no header guard are dropped.
std::vector<const Inclusion *>
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles);
std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ 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"
@@ -186,12 +189,28 @@
}
}
-// 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, ParsedAST &AST) {
+ // 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 = AST.getSourceManager().getFileManager().getFile(
+ AST.getIncludeStructure().getRealPath(
+ static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID)));
+ assert(FE);
+ if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
+ *FE)) {
+ dlog("{0} doesn't have header guard and will not be considered unused",
+ (*FE)->getName());
+ return false;
+ }
+ return true;
}
} // namespace
@@ -223,20 +242,24 @@
}
std::vector<const Inclusion *>
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles) {
std::vector<const Inclusion *> 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<IncludeStructure::HeaderID>(*MFI.HeaderID);
- if (!ReferencedFiles.contains(IncludeID))
+ bool Used = ReferencedFiles.contains(IncludeID);
+ if (!Used && !mayConsiderUnused(MFI, AST)) {
+ dlog("{0} is not a valid unused header and will be filtered out",
+ MFI.Written);
+ continue;
+ }
+ if (!Used)
Unused.push_back(&MFI);
- dlog("{0} is {1}", MFI.Written,
- ReferencedFiles.contains(IncludeID) ? "USED" : "UNUSED");
+ dlog("{0} is {1}", MFI.Written, Used ? "USED" : "UNUSED");
}
return Unused;
}
@@ -272,9 +295,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<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
@@ -291,8 +320,6 @@
->getName()
.str();
for (const auto *Inc : computeUnusedIncludes(AST)) {
- if (!mayConsiderUnused(Inc))
- continue;
Diag D;
D.Message =
llvm::formatv("included header {0} is not used",
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
