hokein updated this revision to Diff 173324.
hokein marked 2 inline comments as done.
hokein added a comment.
Address comments and simplify the code.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54300
Files:
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===================================================================
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -523,6 +523,64 @@
AllOf(QName("Z"), RefCount(0)), QName("y")));
}
+TEST_F(SymbolCollectorTest, ShouldIndexFile) {
+ const std::string IgnoredHeader = R"(
+ class Foo {};
+ void f();
+
+ void k();
+ inline void k() {}
+ #define GLOBAL_Z(name) Z name;
+ )";
+
+ Annotations IndexedHeader(R"(
+ class $bar[[Bar]] {};
+ void $b[[b]]();
+ )");
+ Annotations Main(R"(
+ #include "ignored_header.h"
+ #include "indexed_header.h"
+
+ void $f[[f]]() {}
+ void $b[[b]]() { $f[[f]](); }
+ )");
+ const std::string IgnoredHeaderPath = testPath("ignored_header.h");
+ const std::string IndexedHeaderPath = testPath("indexed_header.h");
+ const std::string IgnoredHeaderURI =
+ URI::createFile(IgnoredHeaderPath).toString();
+ const std::string IndexedHeaderURI =
+ URI::createFile(IndexedHeaderPath).toString();
+
+ InMemoryFileSystem->addFile(IgnoredHeaderPath, 0,
+ MemoryBuffer::getMemBuffer(IgnoredHeader));
+ InMemoryFileSystem->addFile(IndexedHeaderPath, 0,
+ MemoryBuffer::getMemBuffer(IndexedHeader.code()));
+ CollectorOpts.FileFilter = [](const SourceManager &SM, FileID FID) {
+ if (const auto *F = SM.getFileEntryForID(FID))
+ return !F->getName().contains("ignored_header.h");
+ return true;
+ };
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.RefsInHeaders = true;
+ runSymbolCollector("", Main.code());
+ EXPECT_THAT(
+ Symbols,
+ UnorderedElementsAre(
+ // We still see complete symbol f.
+ AllOf(QName("f"), DefURI(TestFileURI), DeclURI(IgnoredHeaderURI)),
+ AllOf(QName("Bar"), DeclRange(IndexedHeader.range("bar")),
+ DeclURI(IndexedHeaderURI)),
+ AllOf(QName("b"), DeclRange(IndexedHeader.range("b")),
+ DefRange(Main.range("b")), DeclURI(IndexedHeaderURI),
+ DefURI(TestFileURI))));
+ EXPECT_THAT(
+ Refs, UnorderedElementsAre(
+ Pair(findSymbol(Symbols, "f").ID, HaveRanges(Main.ranges("f"))),
+ Pair(findSymbol(Symbols, "b").ID, _),
+ Pair(findSymbol(Symbols, "Bar").ID,
+ HaveRanges(IndexedHeader.ranges("bar")))));
+}
+
TEST_F(SymbolCollectorTest, SymbolRelativeNoFallback) {
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols, UnorderedElementsAre(
Index: clangd/index/SymbolCollector.h
===================================================================
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -78,6 +78,9 @@
bool CollectMacro = false;
/// If this is set, only collect symbols/references from a file if
/// `FileFilter(SM, FID)` is true. If not set, all files are indexed.
+ /// Note that a symbol can be redeclared in multiple files, a complete
+ /// symbol will be constructed if this symbol is declared in one of indexed
+ /// files.
std::function<bool(const SourceManager &, FileID)> FileFilter = nullptr;
};
Index: clangd/index/SymbolCollector.cpp
===================================================================
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -214,6 +214,13 @@
return I.first->second;
}
+bool shouldIndexFile(const Decl& D, const SymbolCollector::Options &Opts,
+ llvm::DenseMap<FileID, bool> *FilesToIndexCache) {
+ auto &SM = D.getASTContext().getSourceManager();
+ auto Loc = findNameLoc(&D);
+ return shouldIndexFile(SM, SM.getFileID(Loc), Opts, FilesToIndexCache);
+}
+
// Return the symbol location of the token at \p TokLoc.
Optional<SymbolLocation> getTokenLocation(SourceLocation TokLoc,
const SourceManager &SM,
@@ -353,9 +360,10 @@
// D may not be an interesting symbol, but it's cheaper to check at the end.
auto &SM = ASTCtx->getSourceManager();
auto SpellingLoc = SM.getSpellingLoc(Loc);
+ auto FID = SM.getFileID(SpellingLoc);
if (Opts.CountReferences &&
(Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
- SM.getFileID(SpellingLoc) == SM.getMainFileID())
+ FID == SM.getMainFileID())
ReferencedDecls.insert(ND);
bool CollectRef = static_cast<unsigned>(Opts.RefFilter) & Roles;
@@ -368,8 +376,10 @@
if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
return true;
if (CollectRef && !isa<NamespaceDecl>(ND) &&
- (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
+ (Opts.RefsInHeaders || FID == SM.getMainFileID()) &&
+ shouldIndexFile(SM, FID, Opts, &FilesToIndexCache)) {
DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+ }
// Don't continue indexing if this is a mere reference.
if (IsOnlyRef)
return true;
@@ -380,17 +390,27 @@
const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD);
const Symbol *BasicSymbol = Symbols.find(*ID);
- if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
+ if (!BasicSymbol && shouldIndexFile(*ND, Opts, &FilesToIndexCache)) // Regardless of role, ND is the canonical declaration.
BasicSymbol = addDeclaration(*ND, std::move(*ID));
- else if (isPreferredDeclaration(OriginalDecl, Roles))
+ else if (isPreferredDeclaration(OriginalDecl, Roles) &&
+ shouldIndexFile(OriginalDecl, Opts, &FilesToIndexCache)) {
// If OriginalDecl is preferred, replace the existing canonical
// declaration (e.g. a class forward declaration). There should be at most
// one duplicate as we expect to see only one preferred declaration per
// TU, because in practice they are definitions.
BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID));
+ }
- if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
+ if (Roles & static_cast<unsigned>(index::SymbolRole::Definition) &&
+ shouldIndexFile(OriginalDecl, Opts, &FilesToIndexCache)) {
+ // Here we hit cases: files that declared the symbol are not indexed, but
+ // the file defined the symbol is indexed, we construct a full complete
+ // symbol.
+ if (!BasicSymbol)
+ BasicSymbol = addDeclaration(
+ *cast<NamedDecl>(OriginalDecl.getCanonicalDecl()), *ID);
addDefinition(OriginalDecl, *BasicSymbol);
+ }
return true;
}
@@ -438,8 +458,8 @@
S.Flags |= Symbol::IndexedForCodeCompletion;
S.SymInfo = index::getSymbolInfoForMacro(*MI);
std::string FileURI;
- // FIXME: use the result to filter out symbols.
- shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
+ if (!shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache))
+ return true;
if (auto DeclLoc =
getTokenLocation(DefLoc, SM, Opts, PP->getLangOpts(), FileURI))
S.CanonicalDeclaration = *DeclLoc;
@@ -516,8 +536,6 @@
if (auto ID = getSymbolID(It.first)) {
for (const auto &LocAndRole : It.second) {
auto FileID = SM.getFileID(LocAndRole.first);
- // FIXME: use the result to filter out references.
- shouldIndexFile(SM, FileID, Opts, &FilesToIndexCache);
if (auto FileURI = GetURI(FileID)) {
auto Range =
getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
@@ -558,8 +576,6 @@
S.SymInfo = index::getSymbolInfo(&ND);
std::string FileURI;
auto Loc = findNameLoc(&ND);
- // FIXME: use the result to filter out symbols.
- shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
if (auto DeclLoc =
getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
S.CanonicalDeclaration = *DeclLoc;
@@ -614,8 +630,6 @@
std::string FileURI;
auto Loc = findNameLoc(&ND);
const auto &SM = ND.getASTContext().getSourceManager();
- // FIXME: use the result to filter out symbols.
- shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
if (auto DefLoc =
getTokenLocation(Loc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
S.Definition = *DefLoc;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
