[clang-tools-extra] f43c4c5 - Revert "[clangd] IncludeCleaner: Add support for IWYU pragma private"
Kirill Bobyrev via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 31 09:00:03 PDT 2022
Author: Kirill Bobyrev
Date: 2022-03-31T17:59:52+02:00
New Revision: f43c4c5be29b4111bb953371b8ca83a4511fb1c1
URL: https://github.com/llvm/llvm-project/commit/f43c4c5be29b4111bb953371b8ca83a4511fb1c1
DIFF: https://github.com/llvm/llvm-project/commit/f43c4c5be29b4111bb953371b8ca83a4511fb1c1.diff
LOG: Revert "[clangd] IncludeCleaner: Add support for IWYU pragma private"
This reverts commit 4cb38bfe76b7ef157485338623c931d04d17b958.
Awkwardly enough, this builds Windows buildbots:
http://45.33.8.238/win/55402/step_9.txt
It is yet unclear why this is happening but I will need more time to
diagnose the issue.
Added:
Modified:
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
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 14cc3409cae49..04dbf12410cf7 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -12,7 +12,6 @@
#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"
@@ -24,7 +23,6 @@
#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"
@@ -305,10 +303,9 @@ ReferencedLocations findReferencedLocations(ParsedAST &AST) {
&AST.getTokens());
}
-ReferencedFiles findReferencedFiles(
- const ReferencedLocations &Locs, const SourceManager &SM,
- llvm::function_ref<FileID(FileID)> HeaderResponsible,
- llvm::function_ref<Optional<StringRef>(FileID)> UmbrellaHeader) {
+ReferencedFiles
+findReferencedFiles(const ReferencedLocations &Locs, const SourceManager &SM,
+ llvm::function_ref<FileID(FileID)> HeaderResponsible) {
std::vector<SourceLocation> Sorted{Locs.User.begin(), Locs.User.end()};
llvm::sort(Sorted); // Group by FileID.
ReferencedFilesBuilder Builder(SM);
@@ -327,55 +324,33 @@ ReferencedFiles findReferencedFiles(
// 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) {
+ for (FileID ID : Builder.Files)
UserFiles.insert(HeaderResponsible(ID));
- if (auto PublicHeader = UmbrellaHeader(ID)) {
- PublicHeaders.insert(*PublicHeader);
- }
- }
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),
- std::move(PublicHeaders)};
+ return {std::move(UserFiles), std::move(StdlibFiles)};
}
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);
- },
- [&SM, &CanonIncludes](FileID ID) -> Optional<StringRef> {
- const FileEntry *Entry = SM.getFileEntryForID(ID);
- if (Entry) {
- auto PublicHeader = CanonIncludes.mapHeader(Entry->getName());
- if (!PublicHeader.empty()) {
- return PublicHeader;
- }
- }
- return llvm::None;
- });
+ return findReferencedFiles(Locs, SM, [&SM, &Includes](FileID ID) {
+ return headerResponsible(ID, SM, Includes);
+ });
}
std::vector<const Inclusion *>
getUnused(ParsedAST &AST,
- const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles,
- const llvm::StringSet<> &ReferencedPublicHeaders) {
+ const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles) {
trace::Span Tracer("IncludeCleaner::getUnused");
std::vector<const Inclusion *> Unused;
for (const Inclusion &MFI : AST.getIncludeStructure().MainFileIncludes) {
if (!MFI.HeaderID)
continue;
- if (ReferencedPublicHeaders.contains(MFI.Written))
- continue;
auto IncludeID = static_cast<IncludeStructure::HeaderID>(*MFI.HeaderID);
bool Used = ReferencedFiles.contains(IncludeID);
if (!Used && !mayConsiderUnused(MFI, AST)) {
@@ -425,12 +400,11 @@ std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST) {
const auto &SM = AST.getSourceManager();
auto Refs = findReferencedLocations(AST);
- auto ReferencedFiles =
- findReferencedFiles(Refs, AST.getIncludeStructure(),
- AST.getCanonicalIncludes(), AST.getSourceManager());
+ auto ReferencedFileIDs = findReferencedFiles(Refs, AST.getIncludeStructure(),
+ AST.getSourceManager());
auto ReferencedHeaders =
- translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
- return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
+ translateToHeaderIDs(ReferencedFileIDs, AST.getIncludeStructure(), SM);
+ return getUnused(AST, ReferencedHeaders);
}
std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h
index 4ce31baaa067a..183f84f2f3bfe 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.h
+++ b/clang-tools-extra/clangd/IncludeCleaner.h
@@ -13,6 +13,9 @@
/// 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
@@ -20,12 +23,10 @@
#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 {
@@ -57,11 +58,6 @@ ReferencedLocations findReferencedLocations(ParsedAST &AST);
struct ReferencedFiles {
llvm::DenseSet<FileID> User;
llvm::DenseSet<tooling::stdlib::Header> Stdlib;
- /// Files responsible for the symbols referenced in the main file and defined
- /// in private headers (private headers have IWYU pragma: private, include
- /// "public.h"). We store spelling of the public header (with quotes or angle
- /// brackets) files here to avoid dealing with full filenames and visibility.
- llvm::StringSet<> SpelledUmbrellas;
};
/// Retrieves IDs of all files containing SourceLocations from \p Locs.
@@ -70,16 +66,11 @@ struct ReferencedFiles {
/// \p HeaderResponsible returns the public header that should be included given
/// symbols from a file with the given FileID (example: public headers should be
/// preferred to non self-contained and private headers).
-/// \p UmbrellaHeader returns the public public header is responsible for
-/// providing symbols from a file with the given FileID (example: MyType.h
-/// should be included instead of MyType_impl.h).
-ReferencedFiles findReferencedFiles(
- const ReferencedLocations &Locs, const SourceManager &SM,
- llvm::function_ref<FileID(FileID)> HeaderResponsible,
- llvm::function_ref<Optional<StringRef>(FileID)> UmbrellaHeader);
+ReferencedFiles
+findReferencedFiles(const ReferencedLocations &Locs, const SourceManager &SM,
+ llvm::function_ref<FileID(FileID)> HeaderResponsible);
ReferencedFiles findReferencedFiles(const ReferencedLocations &Locs,
const IncludeStructure &Includes,
- const CanonicalIncludes &CanonIncludes,
const SourceManager &SM);
/// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
@@ -92,8 +83,7 @@ translateToHeaderIDs(const ReferencedFiles &Files,
/// In unclear cases, headers are not marked as unused.
std::vector<const Inclusion *>
getUnused(ParsedAST &AST,
- const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles,
- const llvm::StringSet<> &ReferencedPublicHeaders);
+ const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles);
std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index ffecea5713215..b6bf7695f8873 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -9,7 +9,6 @@
#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"
@@ -281,9 +280,8 @@ TEST(IncludeCleaner, Stdlib) {
ReferencedLocations Locs = findReferencedLocations(AST);
EXPECT_THAT(Locs.Stdlib, ElementsAreArray(WantSyms));
- ReferencedFiles Files =
- findReferencedFiles(Locs, AST.getIncludeStructure(),
- AST.getCanonicalIncludes(), AST.getSourceManager());
+ ReferencedFiles Files = findReferencedFiles(Locs, AST.getIncludeStructure(),
+ AST.getSourceManager());
EXPECT_THAT(Files.Stdlib, ElementsAreArray(WantHeaders));
}
}
@@ -394,8 +392,8 @@ TEST(IncludeCleaner, VirtualBuffers) {
auto &SM = AST.getSourceManager();
auto &Includes = AST.getIncludeStructure();
- auto ReferencedFiles = findReferencedFiles(
- findReferencedLocations(AST), Includes, AST.getCanonicalIncludes(), SM);
+ auto ReferencedFiles =
+ findReferencedFiles(findReferencedLocations(AST), Includes, SM);
llvm::StringSet<> ReferencedFileNames;
for (FileID FID : ReferencedFiles.User)
ReferencedFileNames.insert(
@@ -414,9 +412,7 @@ TEST(IncludeCleaner, VirtualBuffers) {
EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h")));
// Sanity check.
- EXPECT_THAT(
- getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas),
- IsEmpty());
+ EXPECT_THAT(getUnused(AST, ReferencedHeaders), IsEmpty());
}
TEST(IncludeCleaner, DistinctUnguardedInclusions) {
@@ -445,9 +441,9 @@ TEST(IncludeCleaner, DistinctUnguardedInclusions) {
ParsedAST AST = TU.build();
- auto ReferencedFiles = findReferencedFiles(
- findReferencedLocations(AST), AST.getIncludeStructure(),
- AST.getCanonicalIncludes(), AST.getSourceManager());
+ auto ReferencedFiles =
+ findReferencedFiles(findReferencedLocations(AST),
+ AST.getIncludeStructure(), AST.getSourceManager());
llvm::StringSet<> ReferencedFileNames;
auto &SM = AST.getSourceManager();
for (FileID FID : ReferencedFiles.User)
@@ -479,9 +475,9 @@ TEST(IncludeCleaner, NonSelfContainedHeaders) {
ParsedAST AST = TU.build();
- auto ReferencedFiles = findReferencedFiles(
- findReferencedLocations(AST), AST.getIncludeStructure(),
- AST.getCanonicalIncludes(), AST.getSourceManager());
+ auto ReferencedFiles =
+ findReferencedFiles(findReferencedLocations(AST),
+ AST.getIncludeStructure(), AST.getSourceManager());
llvm::StringSet<> ReferencedFileNames;
auto &SM = AST.getSourceManager();
for (FileID FID : ReferencedFiles.User)
@@ -497,31 +493,15 @@ TEST(IncludeCleaner, IWYUPragmas) {
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.getCanonicalIncludes(), AST.getSourceManager());
- EXPECT_EQ(ReferencedFiles.SpelledUmbrellas.size(), 1u);
- EXPECT_EQ(ReferencedFiles.SpelledUmbrellas.begin()->getKey(), "\"public.h\"");
- EXPECT_EQ(ReferencedFiles.User.size(), 2u);
- EXPECT_TRUE(
- ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
- EXPECT_TRUE(
- ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
+ auto ReferencedFiles =
+ findReferencedFiles(findReferencedLocations(AST),
+ AST.getIncludeStructure(), AST.getSourceManager());
+ EXPECT_TRUE(ReferencedFiles.User.empty());
EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
- auto Unused = computeUnusedIncludes(AST);
- EXPECT_THAT(Unused, IsEmpty());
}
} // namespace
More information about the cfe-commits
mailing list