sammccall created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
This updates clangd to take advantage of the APIs added in D155819
<https://reviews.llvm.org/D155819>.
The main difficulties here are around path normalization.
For layering and performance reasons Includes compares paths lexically, and so
we should have consistent paths that can be compared across addSearchPath() and
add(): symlinks resolved or not, relative or absolute.
This patch spells out that requirement, for most tools consistent use of
FileManager/HeaderSearch is enough.
For clangd this does not work: IncludeStructure doesn't hold FileEntrys due to
the preamble/main-file split. It records paths, but canonicalizes them first.
We choose to use this canonical form as our common representation, so we have
to canonicalize the directory entries too. This is done in preamble-build and
recorded in IncludeStructure, as canonicalization is quite expensive.
Left as future work: while include-cleaner correctly uses FileEntry for Header,
as it cares about inode identity, actual Includes have a single path.
They can be modeled with FileEntryRef, and probably should be.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D155878
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
===================================================================
--- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
+++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h
@@ -172,8 +172,20 @@
/// Registers a directory on the include path (-I etc) from HeaderSearch.
/// This allows reasoning about equivalence of e.g. "path/a/b.h" and "a/b.h".
/// This must be called before calling add() in order to take effect.
+ ///
+ /// The paths may be relative or absolute, but the paths passed to
+ /// addSearchDirectory() and add() (that is: Include.Resolved->getName())
+ /// should be consistent, as they are compared lexically.
+ /// Generally, this is satisfied if you obtain paths through HeaderSearch
+ /// and FileEntries through PPCallbacks::IncludeDirective().
+ ///
+ /// FIXME: change Include to use FileEntryRef so callers don't have to worry
+ /// about the behavior of FileEntry::getName() to follow this contract.
void addSearchDirectory(llvm::StringRef);
+ /// Registers an include directive seen in the main file.
+ ///
+ /// This should only be called after all search directories are added.
void add(const Include &);
/// All #includes seen, in the order they appear.
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -459,48 +459,6 @@
MainCode.range());
}
-TEST(IncludeCleaner, FirstMatchedProvider) {
- struct {
- const char *Code;
- const std::vector<include_cleaner::Header> Providers;
- const std::optional<include_cleaner::Header> ExpectedProvider;
- } Cases[] = {
- {R"cpp(
- #include "bar.h"
- #include "foo.h"
- )cpp",
- {include_cleaner::Header{"bar.h"}, include_cleaner::Header{"foo.h"}},
- include_cleaner::Header{"bar.h"}},
- {R"cpp(
- #include "bar.h"
- #include "foo.h"
- )cpp",
- {include_cleaner::Header{"foo.h"}, include_cleaner::Header{"bar.h"}},
- include_cleaner::Header{"foo.h"}},
- {"#include \"bar.h\"",
- {include_cleaner::Header{"bar.h"}},
- include_cleaner::Header{"bar.h"}},
- {"#include \"bar.h\"", {include_cleaner::Header{"foo.h"}}, std::nullopt},
- {"#include \"bar.h\"", {}, std::nullopt}};
- for (const auto &Case : Cases) {
- Annotations Code{Case.Code};
- SCOPED_TRACE(Code.code());
-
- TestTU TU;
- TU.Code = Code.code();
- TU.AdditionalFiles["bar.h"] = "";
- TU.AdditionalFiles["foo.h"] = "";
-
- auto AST = TU.build();
- std::optional<include_cleaner::Header> MatchedProvider =
- firstMatchedProvider(
- convertIncludes(AST.getSourceManager(),
- AST.getIncludeStructure().MainFileIncludes),
- Case.Providers);
- EXPECT_EQ(MatchedProvider, Case.ExpectedProvider);
- }
-}
-
TEST(IncludeCleaner, BatchFix) {
TestTU TU;
TU.Filename = "main.cpp";
@@ -564,6 +522,32 @@
FixMessage("fix all includes")})));
}
+// In the presence of IWYU pragma private, we should accept spellings other
+// than the recommended one if they appear to name the same public header.
+TEST(IncludeCleaner, VerbatimEquivalence) {
+ auto TU = TestTU::withCode(R"cpp(
+ #include "lib/rel/public.h"
+ int x = Public;
+ )cpp");
+ TU.AdditionalFiles["repo/lib/rel/private.h"] = R"cpp(
+ #pragma once
+ // IWYU pragma: private, include "rel/public.h"
+ int Public;
+ )cpp";
+ TU.AdditionalFiles["repo/lib/rel/public.h"] = R"cpp(
+ #pragma once
+ #include "rel/private.h"
+ )cpp";
+
+ TU.ExtraArgs.push_back("-Irepo");
+ TU.ExtraArgs.push_back("-Irepo/lib");
+
+ auto AST = TU.build();
+ auto Findings = computeIncludeCleanerFindings(AST);
+ EXPECT_THAT(Findings.MissingIncludes, IsEmpty());
+ EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -292,6 +292,15 @@
directive(tok::pp_include_next)));
}
+TEST_F(HeadersTest, SearchPath) {
+ FS.Files["foo/bar.h"] = "x";
+ FS.Files["foo/bar/baz.h"] = "y";
+ CDB.ExtraClangFlags.push_back("-Ifoo/bar");
+ CDB.ExtraClangFlags.push_back("-Ifoo/bar/..");
+ EXPECT_THAT(collectIncludes().SearchPathCanonical,
+ ElementsAre(Subdir, testPath("foo/bar"), testPath("foo")));
+}
+
TEST_F(HeadersTest, InsertInclude) {
std::string Path = testPath("sub/bar.h");
FS.Files[Path] = "";
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1327,24 +1327,15 @@
if (IncludeOnLine == Includes.end())
return std::nullopt;
- const auto &Inc = *IncludeOnLine;
const SourceManager &SM = AST.getSourceManager();
ReferencesResult Results;
- auto ConvertedMainFileIncludes = convertIncludes(SM, Includes);
- auto ReferencedInclude = convertIncludes(SM, Inc);
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef<include_cleaner::Header> Providers) {
- if (Ref.RT != include_cleaner::RefType::Explicit)
- return;
-
- auto Provider =
- firstMatchedProvider(ConvertedMainFileIncludes, Providers);
- if (!Provider || ReferencedInclude.match(*Provider).empty())
- return;
+ auto Converted = convertIncludes(AST);
+ auto *Target = Converted.atLine(IncludeOnLine->HashLine + 1);
+ if (!Target)
+ return std::nullopt;
+ auto AddResult =
+ [&](const include_cleaner::SymbolReference &Ref) {
auto Loc = SM.getFileLoc(Ref.RefLocation);
// File locations can be outside of the main file if macro is
// expanded through an #include.
@@ -1357,14 +1348,29 @@
sourceLocToPosition(SM, Token->endLocation())};
Result.Loc.uri = URIMainFile;
Results.References.push_back(std::move(Result));
+ };
+ include_cleaner::walkUsed(
+ AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
+ AST.getPragmaIncludes(), SM,
+ [&](const include_cleaner::SymbolReference &Ref,
+ llvm::ArrayRef<include_cleaner::Header> Providers) {
+ if (Ref.RT != include_cleaner::RefType::Explicit)
+ return;
+ for (const auto &H : Providers) {
+ auto Matches = Converted.match(H);
+ if (llvm::is_contained(Matches, Target))
+ AddResult(Ref);
+ if (!Matches.empty())
+ break;
+ }
});
if (Results.References.empty())
return std::nullopt;
// Add the #include line to the references list.
ReferencesResult::Reference Result;
- Result.Loc.range =
- rangeTillEOL(SM.getBufferData(SM.getMainFileID()), Inc.HashOffset);
+ Result.Loc.range = rangeTillEOL(SM.getBufferData(SM.getMainFileID()),
+ IncludeOnLine->HashOffset);
Result.Loc.uri = URIMainFile;
Results.References.push_back(std::move(Result));
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -72,17 +72,11 @@
/// Converts the clangd include representation to include-cleaner
/// include representation.
-include_cleaner::Includes
-convertIncludes(const SourceManager &SM,
- const llvm::ArrayRef<Inclusion> Includes);
+include_cleaner::Includes convertIncludes(const ParsedAST &);
std::vector<include_cleaner::SymbolReference>
collectMacroReferences(ParsedAST &AST);
-/// Find the first provider in the list that is matched by the includes.
-std::optional<include_cleaner::Header>
-firstMatchedProvider(const include_cleaner::Includes &Includes,
- llvm::ArrayRef<include_cleaner::Header> Providers);
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -26,6 +26,7 @@
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Format/Format.h"
+#include "clang/Lex/DirectoryLookup.h"
#include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Core/Replacement.h"
@@ -353,11 +354,16 @@
return Macros;
}
-include_cleaner::Includes
-convertIncludes(const SourceManager &SM,
- const llvm::ArrayRef<Inclusion> Includes) {
+include_cleaner::Includes convertIncludes(const ParsedAST &AST) {
+ auto &SM = AST.getSourceManager();
+
include_cleaner::Includes ConvertedIncludes;
- for (const Inclusion &Inc : Includes) {
+ // We satisfy Includes's contract that search dirs and included files have
+ // matching path styles: both ultimately use FileManager::getCanonicalName().
+ for (const auto &Dir : AST.getIncludeStructure().SearchPathCanonical)
+ ConvertedIncludes.addSearchDirectory(Dir);
+
+ for (const Inclusion &Inc : AST.getIncludeStructure().MainFileIncludes) {
include_cleaner::Include TransformedInc;
llvm::StringRef WrittenRef = llvm::StringRef(Inc.Written);
TransformedInc.Spelled = WrittenRef.trim("\"<>");
@@ -365,6 +371,10 @@
SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset);
TransformedInc.Line = Inc.HashLine + 1;
TransformedInc.Angled = WrittenRef.starts_with("<");
+ // Inc.Resolved is canonicalized with clangd::getCanonicalPath(),
+ // which is based on FileManager::getCanonicalName(ParentDir).
+ // Calling getFile() updates FileEntry::getName() to match this path.
+ // FIXME: make this explicit by using FileEntryRef instead.
auto FE = SM.getFileManager().getFile(Inc.Resolved);
if (!FE) {
elog("IncludeCleaner: Failed to get an entry for resolved path {0}: {1}",
@@ -382,9 +392,7 @@
if (AST.getLangOpts().ObjC)
return {};
const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- include_cleaner::Includes ConvertedIncludes =
- convertIncludes(SM, Includes.MainFileIncludes);
+ include_cleaner::Includes ConvertedIncludes = convertIncludes(AST);
const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
auto *PreamblePatch = PreamblePatch::getPatchEntry(AST.tuPath(), SM);
@@ -407,7 +415,7 @@
}
for (auto *Inc : ConvertedIncludes.match(H)) {
Satisfied = true;
- auto HeaderID = Includes.getID(Inc->Resolved);
+ auto HeaderID = AST.getIncludeStructure().getID(Inc->Resolved);
assert(HeaderID.has_value() &&
"ConvertedIncludes only contains resolved includes.");
Used.insert(*HeaderID);
@@ -500,14 +508,4 @@
return Result;
}
-std::optional<include_cleaner::Header>
-firstMatchedProvider(const include_cleaner::Includes &Includes,
- llvm::ArrayRef<include_cleaner::Header> Providers) {
- for (const auto &H : Providers) {
- if (!Includes.match(H).empty())
- return H;
- }
- // No match for this provider in the includes list.
- return std::nullopt;
-}
} // namespace clang::clangd
Index: clang-tools-extra/clangd/Hover.cpp
===================================================================
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -1195,8 +1195,7 @@
return;
std::string Result;
- include_cleaner::Includes ConvertedIncludes =
- convertIncludes(SM, AST.getIncludeStructure().MainFileIncludes);
+ include_cleaner::Includes ConvertedIncludes = convertIncludes(AST);
for (const auto &P : RankedProviders) {
if (P.kind() == include_cleaner::Header::Physical &&
P.physical() == SM.getFileEntryForID(SM.getMainFileID()))
@@ -1248,9 +1247,10 @@
void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI, const Inclusion &Inc) {
const SourceManager &SM = AST.getSourceManager();
- const auto &ConvertedMainFileIncludes =
- convertIncludes(SM, AST.getIncludeStructure().MainFileIncludes);
- const auto &HoveredInclude = convertIncludes(SM, llvm::ArrayRef{Inc});
+ const auto &Converted = convertIncludes(AST);
+ const auto *Target = Converted.atLine(Inc.HashLine + 1);
+ if (Target == nullptr) // conversion can skip some broken inclueds
+ return;
llvm::DenseSet<include_cleaner::Symbol> UsedSymbols;
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
@@ -1261,12 +1261,13 @@
UsedSymbols.contains(Ref.Target))
return;
- auto Provider =
- firstMatchedProvider(ConvertedMainFileIncludes, Providers);
- if (!Provider || HoveredInclude.match(*Provider).empty())
- return;
-
- UsedSymbols.insert(Ref.Target);
+ for (const auto &H : Providers) {
+ auto Matches = Converted.match(H);
+ if (llvm::is_contained(Matches, Target))
+ UsedSymbols.insert(Ref.Target);
+ if (!Matches.empty())
+ break;
+ }
});
for (const auto &UsedSymbolDecl : UsedSymbols)
Index: clang-tools-extra/clangd/Headers.h
===================================================================
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -173,6 +173,11 @@
std::vector<Inclusion> MainFileIncludes;
+ // The entries of the header search path. (HeaderSearch::search_dir_range())
+ // Only includes the plain-directory entries (not header maps or frameworks).
+ // All paths are canonical (FileManager::getCanonicalPath()).
+ std::vector<std::string> SearchPathCanonical;
+
// We reserve HeaderID(0) for the main file and will manually check for that
// in getID and getOrCreateID because the UniqueID is not stable when the
// content of the main file changes.
Index: clang-tools-extra/clangd/Headers.cpp
===================================================================
--- clang-tools-extra/clangd/Headers.cpp
+++ clang-tools-extra/clangd/Headers.cpp
@@ -12,6 +12,7 @@
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/DirectoryLookup.h"
#include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/PPCallbacks.h"
#include "clang/Lex/Preprocessor.h"
@@ -178,6 +179,17 @@
MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
auto Collector = std::make_unique<RecordHeaders>(CI, this);
CI.getPreprocessor().addPPCallbacks(std::move(Collector));
+
+ // If we're reusing a preamble, don't repopulate SearchPathCanonical.
+ // The entries will be the same, but canonicalizing to find out is expensive!
+ if (SearchPathCanonical.empty()) {
+ for (const auto &Dir :
+ CI.getPreprocessor().getHeaderSearchInfo().search_dir_range()) {
+ if (Dir.getLookupType() == DirectoryLookup::LT_NormalDir)
+ SearchPathCanonical.emplace_back(
+ SM.getFileManager().getCanonicalName(*Dir.getDirRef()));
+ }
+ }
}
std::optional<IncludeStructure::HeaderID>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
