hokein updated this revision to Diff 173662.
hokein marked 3 inline comments as done.
hokein added a comment.
Update the patch based on offline discussion.
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
@@ -98,6 +98,13 @@
HaveRanges(const std::vector<Range> Ranges) {
return testing::UnorderedPointwise(RefRange(), Ranges);
}
+MATCHER_P(RefsInFiles, Files, "") {
+ for (const Ref &R : arg) {
+ if (!Files.count(R.Location.FileURI))
+ return false;
+ }
+ return true;
+}
class ShouldCollectSymbolTest : public ::testing::Test {
public:
@@ -523,6 +530,82 @@
AllOf(QName("Z"), RefCount(0)), QName("y")));
}
+TEST_F(SymbolCollectorTest, FileFilter) {
+ const std::string NonIndexedHeader = R"(
+ // non-indexed symbols, didn't see any decl/def in indexed files.
+ class Foo1 {};
+ void k();
+ inline void k() {}
+ #define GLOBAL_Z(name) Z name;
+
+ // indexed symbols
+ class Foo2; // def in indexed .h file
+ class Foo3; // def in indexed .cc file
+ void fun1(); // def in indexed .cc file
+ )";
+
+ Annotations IndexedHeader(R"(
+ // indexed symbols
+ class Bar1 {};
+ class Bar2;
+ class Foo2 {};
+ )");
+ Annotations Main(R"(
+ #include "nonindexed_header.h"
+ #include "indexed_header.h"
+
+ class Foo3 {};
+ class Bar2 {};
+ void fun1() {}
+ )");
+ const std::string NonIndexedHeaderPath = testPath("nonindexed_header.h");
+ const std::string IndexedHeaderPath = testPath("indexed_header.h");
+ const std::string NonIndexedHeaderURI =
+ URI::createFile(NonIndexedHeaderPath).toString();
+ const std::string IndexedHeaderURI =
+ URI::createFile(IndexedHeaderPath).toString();
+
+ InMemoryFileSystem->addFile(NonIndexedHeaderPath, 0,
+ MemoryBuffer::getMemBuffer(NonIndexedHeader));
+ 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("nonindexed_header.h");
+ return true;
+ };
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.RefsInHeaders = true;
+ runSymbolCollector("", Main.code());
+
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo2"), DeclURI(IndexedHeaderURI),
+ DefURI(IndexedHeaderURI)),
+ AllOf(QName("Foo3"), DeclURI(NonIndexedHeaderURI),
+ DefURI(TestFileURI)),
+ AllOf(QName("fun1"), DeclURI(NonIndexedHeaderURI),
+ DefURI(TestFileURI)),
+ AllOf(QName("Bar1"), DeclURI(IndexedHeaderURI),
+ DefURI(IndexedHeaderURI)),
+ AllOf(QName("Bar2"), DeclURI(IndexedHeaderURI),
+ DefURI(TestFileURI))));
+ auto Files = [](const std::initializer_list<llvm::StringRef> &Files) {
+ return llvm::StringSet<>(Files);
+ };
+ EXPECT_THAT(Refs,
+ UnorderedElementsAre(
+ Pair(findSymbol(Symbols, "Foo2").ID,
+ RefsInFiles(Files({IndexedHeaderURI}))),
+ Pair(findSymbol(Symbols, "Foo3").ID,
+ RefsInFiles(Files({TestFileURI}))),
+ Pair(findSymbol(Symbols, "fun1").ID,
+ RefsInFiles(Files({TestFileURI}))),
+ Pair(findSymbol(Symbols, "Bar1").ID,
+ RefsInFiles(Files({IndexedHeaderURI}))),
+ Pair(findSymbol(Symbols, "Bar2").ID,
+ RefsInFiles(Files({IndexedHeaderURI, TestFileURI})))));
+}
+
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;
};
@@ -110,8 +113,8 @@
void finish() override;
private:
- const Symbol *addDeclaration(const NamedDecl &, SymbolID);
- void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
+ const Symbol *addDeclaration(const NamedDecl &, SourceLocation, SymbolID);
+ void addDefinition(SourceLocation, const Symbol &DeclSymbol);
// All Symbols collected from the AST.
SymbolSlab::Builder Symbols;
Index: clangd/index/SymbolCollector.cpp
===================================================================
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -353,9 +353,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 +369,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 +383,44 @@
const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD);
const Symbol *BasicSymbol = Symbols.find(*ID);
- if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
- BasicSymbol = addDeclaration(*ND, std::move(*ID));
- else if (isPreferredDeclaration(OriginalDecl, Roles))
- // 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))
- addDefinition(OriginalDecl, *BasicSymbol);
+ auto shouldIndexSymbol = [&](SourceLocation TargetLoc) {
+ return shouldIndexFile(SM, SM.getFileID(TargetLoc), Opts,
+ &FilesToIndexCache);
+ };
+
+ if (!BasicSymbol) {
+ auto DeclSLoc = findNameLoc(ND);
+ if (shouldIndexSymbol(DeclSLoc))
+ // Regardless of role, ND is the canonical declaration.
+ BasicSymbol = addDeclaration(*ND, DeclSLoc, *ID);
+ }
+ if (isPreferredDeclaration(OriginalDecl, Roles)) {
+ auto DeclSLoc = findNameLoc(&OriginalDecl);
+ if (shouldIndexSymbol(DeclSLoc))
+ // 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, DeclSLoc, *ID);
+ }
+
+ if (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) {
+ auto DefLoc = findNameLoc(&OriginalDecl);
+ if (!shouldIndexSymbol(DefLoc) && !BasicSymbol)
+ return true;
+
+ if (!BasicSymbol) {
+ // The symbol is defined in indexed file, but we haven't indexed the
+ // symbolyet because files where the symbol is declared are filtered out,
+ // now we construct a complete symbols from canononical decl (even the
+ // decl is in a filtered file).
+ const auto *CanonicalDecl =
+ cast<NamedDecl>(OriginalDecl.getCanonicalDecl());
+ BasicSymbol =
+ addDeclaration(*CanonicalDecl, findNameLoc(CanonicalDecl), *ID);
+ }
+ addDefinition(DefLoc, *BasicSymbol);
+ }
return true;
}
@@ -438,8 +468,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 +546,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());
@@ -540,6 +568,7 @@
}
const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
+ SourceLocation DeclSLoc,
SymbolID ID) {
auto &Ctx = ND.getASTContext();
auto &SM = Ctx.getSourceManager();
@@ -557,11 +586,8 @@
S.Flags |= Symbol::ImplementationDetail;
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))
+ getTokenLocation(DeclSLoc, SM, Opts, ASTCtx->getLangOpts(), FileURI))
S.CanonicalDeclaration = *DeclLoc;
// Add completion info.
@@ -603,21 +629,17 @@
return Symbols.find(S.ID);
}
-void SymbolCollector::addDefinition(const NamedDecl &ND,
+void SymbolCollector::addDefinition(SourceLocation DefSLoc,
const Symbol &DeclSym) {
if (DeclSym.Definition)
return;
// If we saw some forward declaration, we end up copying the symbol.
// This is not ideal, but avoids duplicating the "is this a definition" check
// in clang::index. We should only see one definition.
Symbol S = DeclSym;
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))
+ if (auto DefLoc = getTokenLocation(DefSLoc, ASTCtx->getSourceManager(), Opts,
+ ASTCtx->getLangOpts(), FileURI))
S.Definition = *DefLoc;
Symbols.insert(S);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
