[clang-tools-extra] f47564e - [clangd] IncludeCleaner: Skip non self-contained headers
Kirill Bobyrev via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 29 08:57:48 PDT 2021
Author: Kirill Bobyrev
Date: 2021-10-29T17:57:31+02:00
New Revision: f47564ea87a502f5fc6320ba5b57e91186751b12
URL: https://github.com/llvm/llvm-project/commit/f47564ea87a502f5fc6320ba5b57e91186751b12
DIFF: https://github.com/llvm/llvm-project/commit/f47564ea87a502f5fc6320ba5b57e91186751b12.diff
LOG: [clangd] IncludeCleaner: Skip non self-contained headers
Headers without include guards might have side effects or can be the files we
don't want to consider (e.g. tablegen ".inc" files). Skip them when translating
headers to the HeaderIDs that we will consider as unused.
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D112695
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 240eb864d087c..eb593ad86ea00 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/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 @@ void findReferencedMacros(ParsedAST &AST, ReferencedLocations &Result) {
}
}
-// 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
@@ -224,21 +243,23 @@ findReferencedFiles(const llvm::DenseSet<SourceLocation> &Locs,
}
std::vector<const Inclusion *>
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles) {
trace::Span Tracer("IncludeCleaner::getUnused");
std::vector<const Inclusion *> Unused;
- for (const Inclusion &MFI : Includes.MainFileIncludes) {
- // FIXME: Skip includes that are not self-contained.
- if (!MFI.HeaderID) {
- elog("File {0} not found.", MFI.Written);
+ for (const Inclusion &MFI : AST.getIncludeStructure().MainFileIncludes) {
+ if (!MFI.HeaderID)
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} was not used, but is not eligible to be diagnosed as unused",
+ 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;
}
@@ -275,9 +296,15 @@ std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST) {
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,
@@ -295,8 +322,6 @@ std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
->getName()
.str();
for (const auto *Inc : computeUnusedIncludes(AST)) {
- if (!mayConsiderUnused(Inc))
- continue;
Diag D;
D.Message =
llvm::formatv("included header {0} is not used",
diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h
index b797e3c8d6f40..a381e40cb2bb9 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.h
+++ b/clang-tools-extra/clangd/IncludeCleaner.h
@@ -61,8 +61,9 @@ translateToHeaderIDs(const llvm::DenseSet<FileID> &Files,
const IncludeStructure &Includes, const SourceManager &SM);
/// Retrieves headers that are referenced from the main file but not used.
+/// In unclear cases, headers are not marked as unused.
std::vector<const Inclusion *>
-getUnused(const IncludeStructure &Includes,
+getUnused(ParsedAST &AST,
const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles);
std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 4d269684b65a0..277b9b181d9b6 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1483,7 +1483,8 @@ TEST(Diagnostics, Tags) {
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 @@ TEST(DiagnosticsTest, IncludeCleaner) {
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
index 895a2074ad2e7..404be5e153864 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -19,6 +19,10 @@ namespace {
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 @@ TEST(IncludeCleaner, GetUnusedHeaders) {
#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 @@ TEST(IncludeCleaner, GetUnusedHeaders) {
// 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 @@ TEST(IncludeCleaner, GetUnusedHeaders) {
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) {
@@ -252,6 +257,9 @@ TEST(IncludeCleaner, VirtualBuffers) {
// 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 @@ TEST(IncludeCleaner, VirtualBuffers) {
#define ab x
#define concat(x, y) x##y
+
+ #endif // MACROS_H
)cpp";
TU.ExtraArgs = {"-DCLI=42"};
ParsedAST AST = TU.build();
@@ -286,7 +296,7 @@ TEST(IncludeCleaner, VirtualBuffers) {
EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
// Sanity check.
- EXPECT_THAT(getUnused(Includes, ReferencedHeaders), ::testing::IsEmpty());
+ EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
}
} // namespace
More information about the cfe-commits
mailing list