kbobyrev updated this revision to Diff 418155.
kbobyrev added a comment.
Fix a bug, add tests for diagnostics. This is ready for a review now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120306/new/
https://reviews.llvm.org/D120306
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 "SourceCode.h"
+#include "TestFS.h"
#include "TestTU.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/Testing/Support/SupportHelpers.h"
@@ -266,8 +267,9 @@
ReferencedLocations Locs = findReferencedLocations(AST);
EXPECT_THAT(Locs.Stdlib, ElementsAreArray(WantSyms));
- ReferencedFiles Files = findReferencedFiles(Locs, AST.getIncludeStructure(),
- AST.getSourceManager());
+ ReferencedFiles Files =
+ findReferencedFiles(Locs, AST.getIncludeStructure(),
+ AST.getCanonicalIncludes(), AST.getSourceManager());
EXPECT_THAT(Files.Stdlib, ElementsAreArray(WantHeaders));
}
}
@@ -378,8 +380,8 @@
auto &SM = AST.getSourceManager();
auto &Includes = AST.getIncludeStructure();
- auto ReferencedFiles =
- findReferencedFiles(findReferencedLocations(AST), Includes, SM);
+ auto ReferencedFiles = findReferencedFiles(
+ findReferencedLocations(AST), Includes, AST.getCanonicalIncludes(), SM);
llvm::StringSet<> ReferencedFileNames;
for (FileID FID : ReferencedFiles.User)
ReferencedFileNames.insert(
@@ -398,7 +400,8 @@
EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
// Sanity check.
- EXPECT_THAT(getUnused(AST, ReferencedHeaders), IsEmpty());
+ EXPECT_THAT(getUnused(AST, ReferencedHeaders, ReferencedFiles.PublicHeaders),
+ IsEmpty());
}
TEST(IncludeCleaner, DistinctUnguardedInclusions) {
@@ -427,9 +430,9 @@
ParsedAST AST = TU.build();
- auto ReferencedFiles =
- findReferencedFiles(findReferencedLocations(AST),
- AST.getIncludeStructure(), AST.getSourceManager());
+ auto ReferencedFiles = findReferencedFiles(
+ findReferencedLocations(AST), AST.getIncludeStructure(),
+ AST.getCanonicalIncludes(), AST.getSourceManager());
llvm::StringSet<> ReferencedFileNames;
auto &SM = AST.getSourceManager();
for (FileID FID : ReferencedFiles.User)
@@ -461,9 +464,9 @@
ParsedAST AST = TU.build();
- auto ReferencedFiles =
- findReferencedFiles(findReferencedLocations(AST),
- AST.getIncludeStructure(), AST.getSourceManager());
+ auto ReferencedFiles = findReferencedFiles(
+ findReferencedLocations(AST), AST.getIncludeStructure(),
+ AST.getCanonicalIncludes(), AST.getSourceManager());
llvm::StringSet<> ReferencedFileNames;
auto &SM = AST.getSourceManager();
for (FileID FID : ReferencedFiles.User)
@@ -479,14 +482,26 @@
TestTU TU;
TU.Code = R"cpp(
#include "behind_keep.h" // IWYU pragma: keep
+ #include "public.h"
+
+ void bar() { foo(); }
)cpp";
TU.AdditionalFiles["behind_keep.h"] = guard("");
+ TU.AdditionalFiles["public.h"] = guard("#include \"private.h\"");
+ TU.AdditionalFiles["private.h"] = guard(R"cpp(
+ // IWYU pragma: private, include "public.h"
+ void foo() {}
+ )cpp");
ParsedAST AST = TU.build();
- auto ReferencedFiles =
- findReferencedFiles(findReferencedLocations(AST),
- AST.getIncludeStructure(), AST.getSourceManager());
- EXPECT_TRUE(ReferencedFiles.User.empty());
+ auto ReferencedFiles = findReferencedFiles(
+ findReferencedLocations(AST), AST.getIncludeStructure(),
+ AST.getCanonicalIncludes(), AST.getSourceManager());
+ EXPECT_EQ(ReferencedFiles.PublicHeaders.size(), 1u);
+ EXPECT_EQ(ReferencedFiles.PublicHeaders.begin()->getKey(), "\"public.h\"");
+ EXPECT_EQ(ReferencedFiles.User.size(), 1u);
+ EXPECT_EQ(*ReferencedFiles.User.begin(),
+ AST.getSourceManager().getMainFileID());
EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
}
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1616,10 +1616,15 @@
]]
#include "used.h"
+$fix_private[[ $diag_private[[#include "private.h"]]
+]]
+ #include "public.h"
+
#include <system_header.h>
void foo() {
used();
+ foo();
}
)cpp");
TestTU TU;
@@ -1632,6 +1637,15 @@
#pragma once
void used() {}
)cpp";
+ TU.AdditionalFiles["public.h"] = R"cpp(
+ #pragma once
+ #include "private.h"
+ )cpp";
+ TU.AdditionalFiles["private.h"] = R"cpp(
+ // IWYU pragma: private, include "public.h"
+ #pragma once
+ void foo();
+ )cpp";
TU.AdditionalFiles["system/system_header.h"] = "";
TU.ExtraArgs = {"-isystem" + testPath("system")};
// Off by default.
@@ -1641,10 +1655,16 @@
WithContextValue WithCfg(Config::Key, std::move(Cfg));
EXPECT_THAT(
*TU.build().getDiagnostics(),
- UnorderedElementsAre(AllOf(
- Diag(Test.range("diag"), "included header unused.h is not used"),
- withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
- withFix(Fix(Test.range("fix"), "", "remove #include directive")))));
+ UnorderedElementsAre(
+ AllOf(
+ Diag(Test.range("diag"), "included header unused.h is not used"),
+ withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
+ withFix(Fix(Test.range("fix"), "", "remove #include directive"))),
+ AllOf(Diag(Test.range("diag_private"),
+ "included header private.h is not used"),
+ withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
+ withFix(Fix(Test.range("fix_private"), "",
+ "remove #include directive")))));
Cfg.Diagnostics.SuppressAll = true;
WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg));
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -13,9 +13,6 @@
/// to provide useful warnings in most popular scenarios but not 1:1 exact
/// feature compatibility.
///
-/// FIXME(kirillbobyrev): Add support for IWYU pragmas.
-/// FIXME(kirillbobyrev): Add support for standard library headers.
-///
//===----------------------------------------------------------------------===//
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDECLEANER_H
@@ -23,10 +20,12 @@
#include "Headers.h"
#include "ParsedAST.h"
+#include "index/CanonicalIncludes.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/STLFunctionalExtras.h"
+#include "llvm/ADT/StringSet.h"
#include <vector>
namespace clang {
@@ -59,6 +58,7 @@
struct ReferencedFiles {
llvm::DenseSet<FileID> User;
llvm::DenseSet<tooling::stdlib::Header> Stdlib;
+ llvm::StringSet<> PublicHeaders;
};
/// Retrieves IDs of all files containing SourceLocations from \p Locs.
@@ -69,9 +69,12 @@
/// preferred to non self-contained and private headers).
ReferencedFiles
findReferencedFiles(const ReferencedLocations &Locs, const SourceManager &SM,
- llvm::function_ref<FileID(FileID)> HeaderResponsible);
+ llvm::function_ref<void(FileID, llvm::DenseSet<FileID> &,
+ llvm::StringSet<> &)>
+ AddFile);
ReferencedFiles findReferencedFiles(const ReferencedLocations &Locs,
const IncludeStructure &Includes,
+ const CanonicalIncludes &CanonIncludes,
const SourceManager &SM);
/// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
@@ -84,7 +87,8 @@
/// In unclear cases, headers are not marked as unused.
std::vector<const Inclusion *>
getUnused(ParsedAST &AST,
- const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles);
+ const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles,
+ const llvm::StringSet<> &PublicHeaders);
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
@@ -12,6 +12,7 @@
#include "ParsedAST.h"
#include "Protocol.h"
#include "SourceCode.h"
+#include "index/CanonicalIncludes.h"
#include "support/Logger.h"
#include "support/Trace.h"
#include "clang/AST/ASTContext.h"
@@ -23,6 +24,7 @@
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Syntax/Tokens.h"
#include "llvm/ADT/STLFunctionalExtras.h"
+#include "llvm/ADT/StringSet.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Path.h"
@@ -307,7 +309,9 @@
ReferencedFiles
findReferencedFiles(const ReferencedLocations &Locs, const SourceManager &SM,
- llvm::function_ref<FileID(FileID)> HeaderResponsible) {
+ llvm::function_ref<void(FileID, llvm::DenseSet<FileID> &,
+ llvm::StringSet<> &)>
+ AddFile) {
std::vector<SourceLocation> Sorted{Locs.User.begin(), Locs.User.end()};
llvm::sort(Sorted); // Group by FileID.
ReferencedFilesBuilder Builder(SM);
@@ -326,33 +330,52 @@
// non-self-contained FileIDs as used. Perform this on FileIDs rather than
// HeaderIDs, as each inclusion of a non-self-contained file is distinct.
llvm::DenseSet<FileID> UserFiles;
+ llvm::StringSet<> PublicHeaders;
for (FileID ID : Builder.Files)
- UserFiles.insert(HeaderResponsible(ID));
+ AddFile(ID, UserFiles, PublicHeaders);
llvm::DenseSet<tooling::stdlib::Header> StdlibFiles;
for (const auto &Symbol : Locs.Stdlib)
for (const auto &Header : Symbol.headers())
StdlibFiles.insert(Header);
- return {std::move(UserFiles), std::move(StdlibFiles)};
+ return {std::move(UserFiles), std::move(StdlibFiles),
+ std::move(PublicHeaders)};
}
ReferencedFiles findReferencedFiles(const ReferencedLocations &Locs,
const IncludeStructure &Includes,
+ const CanonicalIncludes &CanonIncludes,
const SourceManager &SM) {
- return findReferencedFiles(Locs, SM, [&SM, &Includes](FileID ID) {
- return headerResponsible(ID, SM, Includes);
- });
+ return findReferencedFiles(
+ Locs, SM,
+ [&SM, &Includes, &CanonIncludes](FileID ID,
+ llvm::DenseSet<FileID> &UserFiles,
+ llvm::StringSet<> &PublicHeaders) {
+ auto FID = headerResponsible(ID, SM, Includes);
+ const FileEntry *Entry = SM.getFileEntryForID(FID);
+ if (Entry) {
+ auto PublicHeader = CanonIncludes.mapHeader(Entry->getName());
+ if (!PublicHeader.empty()) {
+ PublicHeaders.insert(PublicHeader);
+ return;
+ }
+ }
+ UserFiles.insert(FID);
+ });
}
std::vector<const Inclusion *>
getUnused(ParsedAST &AST,
- const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles) {
+ const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles,
+ const llvm::StringSet<> &PublicHeaders) {
trace::Span Tracer("IncludeCleaner::getUnused");
std::vector<const Inclusion *> Unused;
for (const Inclusion &MFI : AST.getIncludeStructure().MainFileIncludes) {
if (!MFI.HeaderID)
continue;
+ if (PublicHeaders.contains(MFI.Written))
+ continue;
auto IncludeID = static_cast<IncludeStructure::HeaderID>(*MFI.HeaderID);
bool Used = ReferencedFiles.contains(IncludeID);
if (!Used && !mayConsiderUnused(MFI, AST)) {
@@ -402,11 +425,12 @@
const auto &SM = AST.getSourceManager();
auto Refs = findReferencedLocations(AST);
- auto ReferencedFileIDs = findReferencedFiles(Refs, AST.getIncludeStructure(),
- AST.getSourceManager());
+ auto ReferencedFiles =
+ findReferencedFiles(Refs, AST.getIncludeStructure(),
+ AST.getCanonicalIncludes(), AST.getSourceManager());
auto ReferencedHeaders =
- translateToHeaderIDs(ReferencedFileIDs, AST.getIncludeStructure(), SM);
- return getUnused(AST, ReferencedHeaders);
+ translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
+ return getUnused(AST, ReferencedHeaders, ReferencedFiles.PublicHeaders);
}
std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
