kbobyrev updated this revision to Diff 383085.
kbobyrev marked 4 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;
@@ -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
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.
+/// 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"
@@ -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,32 @@
}
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) {
+ for (const Inclusion &MFI : AST.getIncludeStructure().MainFileIncludes) {
// FIXME: Skip includes that are not self-contained.
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) {
+ // 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 +290,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,
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
