[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE341242: [clangd] Implement findOccurrences interface in
dynamic index. (authored by hokein, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D51279?vs=163539&id=163580#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
clangd/index/Merge.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/TestTU.cpp
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -16,9 +16,10 @@
namespace clang {
namespace clangd {
-SymbolSlab indexAST(ASTContext &AST, std::shared_ptr PP,
-llvm::Optional> TopLevelDecls,
-llvm::ArrayRef URISchemes) {
+std::pair
+indexAST(ASTContext &AST, std::shared_ptr PP,
+ llvm::Optional> TopLevelDecls,
+ llvm::ArrayRef URISchemes) {
SymbolCollector::Options CollectorOpts;
// FIXME(ioeric): we might also want to collect include headers. We would need
// to make sure all includes are canonicalized (with CanonicalIncludes), which
@@ -31,8 +32,6 @@
CollectorOpts.URISchemes = URISchemes;
CollectorOpts.Origin = SymbolOrigin::Dynamic;
- SymbolCollector Collector(std::move(CollectorOpts));
- Collector.setPreprocessor(PP);
index::IndexingOptions IndexOpts;
// We only need declarations, because we don't count references.
IndexOpts.SystemSymbolFilter =
@@ -46,20 +45,45 @@
DeclsToIndex.assign(AST.getTranslationUnitDecl()->decls().begin(),
AST.getTranslationUnitDecl()->decls().end());
+ // We only collect occurrences when indexing main AST.
+ // FIXME: this is a hacky way to detect whether we are indexing preamble AST
+ // or main AST, we should make it explicitly.
+ bool IsIndexMainAST = TopLevelDecls.hasValue();
+ if (IsIndexMainAST)
+CollectorOpts.OccurrenceFilter = AllOccurrenceKinds;
+
+ SymbolCollector Collector(std::move(CollectorOpts));
+ Collector.setPreprocessor(PP);
index::indexTopLevelDecls(AST, DeclsToIndex, Collector, IndexOpts);
- return Collector.takeSymbols();
+ const auto &SM = AST.getSourceManager();
+ const auto *MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+ std::string FileName = MainFileEntry ? MainFileEntry->getName() : "";
+
+ auto Syms = Collector.takeSymbols();
+ auto Occurrences = Collector.takeOccurrences();
+ vlog("index {0}AST for {1}: \n"
+ " symbol slab: {2} symbols, {3} bytes\n"
+ " occurrence slab: {4} symbols, {5} bytes",
+ IsIndexMainAST ? "Main" : "Preamble", FileName, Syms.size(),
+ Syms.bytes(), Occurrences.size(), Occurrences.bytes());
+ return {std::move(Syms), std::move(Occurrences)};
}
FileIndex::FileIndex(std::vector URISchemes)
: URISchemes(std::move(URISchemes)) {}
-void FileSymbols::update(PathRef Path, std::unique_ptr Slab) {
+void FileSymbols::update(PathRef Path, std::unique_ptr Slab,
+ std::unique_ptr Occurrences) {
std::lock_guard Lock(Mutex);
if (!Slab)
FileToSlabs.erase(Path);
else
FileToSlabs[Path] = std::move(Slab);
+ if (!Occurrences)
+FileToOccurrenceSlabs.erase(Path);
+ else
+FileToOccurrenceSlabs[Path] = std::move(Occurrences);
}
std::shared_ptr> FileSymbols::allSymbols() {
@@ -85,19 +109,47 @@
return {std::move(Snap), Pointers};
}
+std::shared_ptr FileSymbols::allOccurrences() const {
+ // The snapshot manages life time of symbol occurrence slabs and provides
+ // pointers to all occurrences in all occurrence slabs.
+ struct Snapshot {
+MemIndex::OccurrenceMap Occurrences; // ID => {Occurrence}
+std::vector> KeepAlive;
+ };
+
+ auto Snap = std::make_shared();
+ {
+std::lock_guard Lock(Mutex);
+
+for (const auto &FileAndSlab : FileToOccurrenceSlabs) {
+ Snap->KeepAlive.push_back(FileAndSlab.second);
+ for (const auto &IDAndOccurrences : *FileAndSlab.second) {
+auto &Occurrences = Snap->Occurrences[IDAndOccurrences.first];
+for (const auto &Occurrence : IDAndOccurrences.second)
+ Occurrences.push_back(&Occurrence);
+ }
+}
+ }
+
+ return {std::move(Snap), &Snap->Occurrences};
+}
+
void FileIndex::update(PathRef Path, ASTContext *AST,
std::shared_ptr PP,
llvm::Optional> TopLevelDecls) {
if (!AST) {
-FSymbols.update(Path, nullptr);
+FSymbols.update(Path, nullptr, nullptr);
} else {
assert(PP);
auto Slab = llvm::make_unique();
-*Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes);
-FSymbols.update(Path, std::move(Slab));
+auto
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein updated this revision to Diff 163539.
hokein marked 3 inline comments as done.
hokein added a comment.
address review comments:
- build memindex with symbol slab and occurrence slab
- remove withAllCode in TestTU
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
clangd/index/Merge.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -45,11 +45,12 @@
SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
- return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
+ return indexAST(AST.getASTContext(), AST.getPreprocessorPtr()).first;
}
std::unique_ptr TestTU::index() const {
- return MemIndex::build(headerSymbols());
+ // FIXME: we should generate proper occurrences for TestTU.
+ return MemIndex::build(headerSymbols(), SymbolOccurrenceSlab::createEmpty());
}
const Symbol &findSymbol(const SymbolSlab &Slab, llvm::StringRef QName) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -7,21 +7,36 @@
//
//===--===//
+#include "Annotations.h"
#include "TestIndex.h"
+#include "TestTU.h"
+#include "gmock/gmock.h"
+#include "index/FileIndex.h"
#include "index/Index.h"
#include "index/MemIndex.h"
#include "index/Merge.h"
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
using testing::Pointee;
using testing::UnorderedElementsAre;
+using testing::AllOf;
namespace clang {
namespace clangd {
namespace {
+std::shared_ptr emptyOccurrences() {
+ return llvm::make_unique();
+}
+
MATCHER_P(Named, N, "") { return arg.Name == N; }
+MATCHER_P(OccurrenceRange, Range, "") {
+ return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
+ arg.Location.End.Line, arg.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+ Range.end.character);
+}
+MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
TEST(SymbolSlab, FindAndIterate) {
SymbolSlab::Builder B;
@@ -42,14 +57,14 @@
TEST(MemIndexTest, MemIndexSymbolsRecycled) {
MemIndex I;
std::weak_ptr Symbols;
- I.build(generateNumSymbols(0, 10, &Symbols));
+ I.build(generateNumSymbols(0, 10, &Symbols), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "7";
EXPECT_THAT(match(I, Req), UnorderedElementsAre("7"));
EXPECT_FALSE(Symbols.expired());
// Release old symbols.
- I.build(generateNumSymbols(0, 0));
+ I.build(generateNumSymbols(0, 0), emptyOccurrences());
EXPECT_TRUE(Symbols.expired());
}
@@ -65,14 +80,14 @@
FuzzyFindRequest Req;
Req.Query = "7";
MemIndex I;
- I.build(std::move(Symbols));
+ I.build(std::move(Symbols), emptyOccurrences());
auto Matches = match(I, Req);
EXPECT_EQ(Matches.size(), 1u);
}
TEST(MemIndexTest, MemIndexLimitedNumMatches) {
MemIndex I;
- I.build(generateNumSymbols(0, 100));
+ I.build(generateNumSymbols(0, 100), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "5";
Req.MaxCandidateCount = 3;
@@ -85,7 +100,8 @@
TEST(MemIndexTest, FuzzyMatch) {
MemIndex I;
I.build(
- generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}));
+ generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}),
+ emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "lol";
Req.MaxCandidateCount = 2;
@@ -95,60 +111,62 @@
TEST(MemIndexTest, MatchQualifiedNamesWithoutSpecificScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "b::y2", "y3"}));
+ I.build(generateSymbols({"a::y1", "b::y2", "y3"}), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "y";
EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3"));
}
TEST(MemIndexTest, MatchQualifiedNamesWithGlobalScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "b::y2", "y3"}));
+ I.build(generateSymbols({"a::y1", "b::y2", "y3"}), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "y";
Req.Scopes = {""};
EXPECT_THAT(match(I, Req), UnorderedElementsAre("y3"));
}
TEST(MemIndexTest, MatchQualifiedNamesWithOneScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}));
+ I.build(generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}),
+ emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "y";
Req.Scopes = {"a::"};
EXPECT_THAT(match(I, Req),
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
sammccall added inline comments.
Comment at: unittests/clangd/TestTU.h:41
+ static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+llvm::StringRef Filename = "") {
hokein wrote:
> sammccall wrote:
> > We've avoided adding this in the past because it's less readable. Please
> > assign the fields separately instead.
> I'm tempted to keep it here, while it's less readable, it does more things,
> and can make client code more readable (see newly-added tests), otherwise we
> have to repeat these statements in a few places.
That's what I mean, I find the newly-added tests hard to read, because
"allcode" isn't clear (particularly when one of the params is a filename) and
the order of parameters is not obvious.
I think they would be much clearer with the fields assigned individually as the
existing tests do.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein updated this revision to Diff 163532.
hokein added a comment.
Minor cleanup.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
clangd/index/Merge.h
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -38,6 +38,16 @@
return TU;
}
+ static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+llvm::StringRef Filename) {
+TestTU TU;
+TU.HeaderCode = HeaderCode;
+TU.Code = Code;
+if (!Filename.empty())
+ TU.Filename = Filename;
+return TU;
+ }
+
// The code to be compiled.
std::string Code;
std::string Filename = "TestTU.cpp";
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -45,7 +45,7 @@
SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
- return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
+ return indexAST(AST.getASTContext(), AST.getPreprocessorPtr()).first;
}
std::unique_ptr TestTU::index() const {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -7,21 +7,36 @@
//
//===--===//
+#include "Annotations.h"
#include "TestIndex.h"
+#include "TestTU.h"
+#include "gmock/gmock.h"
+#include "index/FileIndex.h"
#include "index/Index.h"
#include "index/MemIndex.h"
#include "index/Merge.h"
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
using testing::Pointee;
using testing::UnorderedElementsAre;
+using testing::AllOf;
namespace clang {
namespace clangd {
namespace {
+std::shared_ptr emptyOccurrences() {
+ return llvm::make_unique();
+}
+
MATCHER_P(Named, N, "") { return arg.Name == N; }
+MATCHER_P(OccurrenceRange, Range, "") {
+ return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
+ arg.Location.End.Line, arg.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+ Range.end.character);
+}
+MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
TEST(SymbolSlab, FindAndIterate) {
SymbolSlab::Builder B;
@@ -42,14 +57,14 @@
TEST(MemIndexTest, MemIndexSymbolsRecycled) {
MemIndex I;
std::weak_ptr Symbols;
- I.build(generateNumSymbols(0, 10, &Symbols));
+ I.build(generateNumSymbols(0, 10, &Symbols), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "7";
EXPECT_THAT(match(I, Req), UnorderedElementsAre("7"));
EXPECT_FALSE(Symbols.expired());
// Release old symbols.
- I.build(generateNumSymbols(0, 0));
+ I.build(generateNumSymbols(0, 0), emptyOccurrences());
EXPECT_TRUE(Symbols.expired());
}
@@ -65,14 +80,14 @@
FuzzyFindRequest Req;
Req.Query = "7";
MemIndex I;
- I.build(std::move(Symbols));
+ I.build(std::move(Symbols), emptyOccurrences());
auto Matches = match(I, Req);
EXPECT_EQ(Matches.size(), 1u);
}
TEST(MemIndexTest, MemIndexLimitedNumMatches) {
MemIndex I;
- I.build(generateNumSymbols(0, 100));
+ I.build(generateNumSymbols(0, 100), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "5";
Req.MaxCandidateCount = 3;
@@ -85,7 +100,8 @@
TEST(MemIndexTest, FuzzyMatch) {
MemIndex I;
I.build(
- generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}));
+ generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}),
+ emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "lol";
Req.MaxCandidateCount = 2;
@@ -95,60 +111,62 @@
TEST(MemIndexTest, MatchQualifiedNamesWithoutSpecificScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "b::y2", "y3"}));
+ I.build(generateSymbols({"a::y1", "b::y2", "y3"}), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "y";
EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3"));
}
TEST(MemIndexTest, MatchQualifiedNamesWithGlobalScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "b::y2", "y3"}));
+ I.build(generateSymbols({"a::y1", "b::y2", "y3"}), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "y";
Req.Scopes = {""};
EXPECT_THAT(match(I, Req), UnorderedElementsAre("y3"));
}
TEST(MemIndexTest, MatchQualifiedNamesWithOneScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"}));
+ I.build(generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"})
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein added inline comments.
Comment at: clangd/index/MemIndex.cpp:35
std::unique_ptr MemIndex::build(SymbolSlab Slab) {
auto Idx = llvm::make_unique();
sammccall wrote:
> This is still implicitly creating an index with no occurrences. Did you mean
> to accept a SymbolOccurrencesSlab here?
This is intended, this function only cares about symbol, no occurrences.
Comment at: unittests/clangd/TestTU.h:41
+ static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+llvm::StringRef Filename = "") {
sammccall wrote:
> We've avoided adding this in the past because it's less readable. Please
> assign the fields separately instead.
I'm tempted to keep it here, while it's less readable, it does more things, and
can make client code more readable (see newly-added tests), otherwise we have
to repeat these statements in a few places.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein updated this revision to Diff 163531.
hokein marked 4 inline comments as done.
hokein added a comment.
- rebase
- address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
clangd/index/Merge.h
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -38,6 +38,16 @@
return TU;
}
+ static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+llvm::StringRef Filename) {
+TestTU TU;
+TU.HeaderCode = HeaderCode;
+TU.Code = Code;
+if (!Filename.empty())
+ TU.Filename = Filename;
+return TU;
+ }
+
// The code to be compiled.
std::string Code;
std::string Filename = "TestTU.cpp";
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -45,7 +45,7 @@
SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
- return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
+ return indexAST(AST.getASTContext(), AST.getPreprocessorPtr()).first;
}
std::unique_ptr TestTU::index() const {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -7,21 +7,36 @@
//
//===--===//
+#include "Annotations.h"
#include "TestIndex.h"
+#include "TestTU.h"
+#include "gmock/gmock.h"
+#include "index/FileIndex.h"
#include "index/Index.h"
#include "index/MemIndex.h"
#include "index/Merge.h"
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
using testing::Pointee;
using testing::UnorderedElementsAre;
+using testing::AllOf;
namespace clang {
namespace clangd {
namespace {
+std::shared_ptr emptyOccurrences() {
+ return llvm::make_unique();
+}
+
MATCHER_P(Named, N, "") { return arg.Name == N; }
+MATCHER_P(OccurrenceRange, Range, "") {
+ return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
+ arg.Location.End.Line, arg.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+ Range.end.character);
+}
+MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
TEST(SymbolSlab, FindAndIterate) {
SymbolSlab::Builder B;
@@ -42,14 +57,14 @@
TEST(MemIndexTest, MemIndexSymbolsRecycled) {
MemIndex I;
std::weak_ptr Symbols;
- I.build(generateNumSymbols(0, 10, &Symbols));
+ I.build(generateNumSymbols(0, 10, &Symbols), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "7";
EXPECT_THAT(match(I, Req), UnorderedElementsAre("7"));
EXPECT_FALSE(Symbols.expired());
// Release old symbols.
- I.build(generateNumSymbols(0, 0));
+ I.build(generateNumSymbols(0, 0), emptyOccurrences());
EXPECT_TRUE(Symbols.expired());
}
@@ -65,14 +80,14 @@
FuzzyFindRequest Req;
Req.Query = "7";
MemIndex I;
- I.build(std::move(Symbols));
+ I.build(std::move(Symbols), emptyOccurrences());
auto Matches = match(I, Req);
EXPECT_EQ(Matches.size(), 1u);
}
TEST(MemIndexTest, MemIndexLimitedNumMatches) {
MemIndex I;
- I.build(generateNumSymbols(0, 100));
+ I.build(generateNumSymbols(0, 100), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "5";
Req.MaxCandidateCount = 3;
@@ -85,7 +100,8 @@
TEST(MemIndexTest, FuzzyMatch) {
MemIndex I;
I.build(
- generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}));
+ generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"}),
+ emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "lol";
Req.MaxCandidateCount = 2;
@@ -95,60 +111,62 @@
TEST(MemIndexTest, MatchQualifiedNamesWithoutSpecificScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "b::y2", "y3"}));
+ I.build(generateSymbols({"a::y1", "b::y2", "y3"}), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "y";
EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3"));
}
TEST(MemIndexTest, MatchQualifiedNamesWithGlobalScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "b::y2", "y3"}));
+ I.build(generateSymbols({"a::y1", "b::y2", "y3"}), emptyOccurrences());
FuzzyFindRequest Req;
Req.Query = "y";
Req.Scopes = {""};
EXPECT_THAT(match(I, Req), UnorderedElementsAre("y3"));
}
TEST(MemIndexTest, MatchQualifiedNamesWithOneScope) {
MemIndex I;
- I.build(generateSymbols({"a::y1", "a::y2", "a::x", "
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein marked 3 inline comments as done. hokein added a comment. Sorry! Just realised I messed up this patch with https://reviews.llvm.org/D50385 (mostly SymbolCollector changes), all the comments about `SymbolCollector` are fixed in https://reviews.llvm.org/D50385. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
This basically looks good to go (some fixes needed but they're pretty clear I
think let me know if not!)
Comment at: clangd/index/FileIndex.cpp:63
+ auto Occurrences = Collector.takeOccurrences();
+ vlog("index for AST: \n"
+ " symbol slab: {0} symbols, {1} bytes\n"
for this log to be useful, you might want to include the filename.
You can get it from the sourcemanager on the ASTContext.
Comment at: clangd/index/MemIndex.cpp:35
std::unique_ptr MemIndex::build(SymbolSlab Slab) {
auto Idx = llvm::make_unique();
This is still implicitly creating an index with no occurrences. Did you mean to
accept a SymbolOccurrencesSlab here?
Comment at: clangd/index/MemIndex.h:23
public:
- /// \brief (Re-)Build index for `Symbols`. All symbol pointers must remain
- /// accessible as long as `Symbols` is kept alive.
- void build(std::shared_ptr> Symbols);
+ using OccurrenceMap =
+ llvm::DenseMap>;
can you add an explanation to this?
Comment at: clangd/index/Merge.cpp:94
+// instead.
+llvm::DenseSet DynamicIndexFileURIs;
+Dynamic->findOccurrences(Req, [&](const SymbolOccurrence &O) {
Unfortunately this is not safe, the backing strings may not live long enough.
This should be a StringSet (by value) instead.
Comment at: clangd/index/Merge.cpp:100
+Static->findOccurrences(Req, [&](const SymbolOccurrence &O) {
+ if (DynamicIndexFileURIs.find(O.Location.FileURI) !=
+ DynamicIndexFileURIs.end())
or just DynamicIndexFileURIs.count(O.Location.FileURI) and rely on the implicit
conversion to bool
Comment at: clangd/index/SymbolCollector.cpp:227
+SymbolOccurrenceKind ToOccurrenceKind(index::SymbolRoleSet Roles) {
+ SymbolOccurrenceKind Kind;
toOccurrenceKind
isn't this just
`return Roles & AllOccurrenceKinds` with some casts?
(I'd put AllOccurrenceKinds into the header, but it could also be a constant
here)
Comment at: clangd/index/SymbolCollector.h:120
+
+ using DeclOccurrence = std::pair;
+ llvm::DenseMap>
DeclOccurrences;
move these up next to ReferencedDecls and ReferencedMacros so the comment
applies to them too?
Similarly, move SymbolOccurrenceSlab next to the SymbolSlab::Builder? These
have strong parallels.
Comment at: clangd/index/SymbolCollector.h:122
+ llvm::DenseMap>
DeclOccurrences;
+ // All symbol occurrences collected from the AST, assembled on finish().
+ // Only symbols declared in preamble (from #inclues) and references from the
I'm not sure this works - IIRC can use a SymbolCollector for multiple TUs, with
finish() called at the end of each one.
I think you need to (incrementally) build in finish(), and freeze in
takeOccurrences().
Comment at: unittests/clangd/TestTU.h:41
+ static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+llvm::StringRef Filename = "") {
We've avoided adding this in the past because it's less readable. Please assign
the fields separately instead.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
ilya-biryukov added inline comments.
Comment at: clangd/index/FileIndex.cpp:48
+ if (TopLevelDecls) { // index main AST, set occurrence flag.
+CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
hokein wrote:
> ilya-biryukov wrote:
> > There is an implicit assumption (`TopLevelDecls != null` iff indexing the
> > main file AST in dynamic index) that does not seem quite obvious.
> > Maybe add two overloads (for indexing the main AST and for indexing the
> > preamble AST) or add an extra parameter to be explicit more about those
> > choices?
> This is a hacky way to detect whether we are indexing main AST or preamble
> AST -- because the current `FileIndex::update(PathRef Path, ...)` interface
> doesn't contain this information, and we use it both for `PreambleAST` and
> `MainAST` indexing in the client side (`ParsingCallback`).
>
> I think we might want two dedicated methods (`updateMainAST`, and
> `updatePreambleAST`) in `FileSymbol`, and I think we should address it in a
> follow-up patch. WDYT?
Either a dedicated methods or a separate parameter LG.
And it's probably ok to do it with a follow-up change if you want to keep the
scope of this patch smaller. That should be a pretty small change overall, so
it should be fine either way.
Comment at: clangd/index/FileIndex.h:100
/// level decls obtained from \p AST are indexed.
-SymbolSlab
+std::pair
indexAST(ASTContext &AST, std::shared_ptr PP,
hokein wrote:
> ilya-biryukov wrote:
> > Maybe use a struct to allow named fields? Would mean a tiny amount of extra
> > code in `indexAST`, but should make the client code slightly nicer.
> I think the type of `pair` explicitly express its meaning well enough? And
> this function is exposed for unittests, I'm happy to do the change if you
> insist.
Not really, that was just a suggestion. Feel free to ignore if the current
version looks better to you.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein updated this revision to Diff 163064.
hokein marked 16 inline comments as done.
hokein added a comment.
Herald added a subscriber: mgrang.
Address review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
clangd/index/Merge.h
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -38,6 +38,16 @@
return TU;
}
+ static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+llvm::StringRef Filename = "") {
+TestTU TU;
+TU.HeaderCode = HeaderCode;
+TU.Code = Code;
+if (!Filename.empty())
+ TU.Filename = Filename;
+return TU;
+ }
+
// The code to be compiled.
std::string Code;
std::string Filename = "TestTU.cpp";
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -45,7 +45,7 @@
SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
- return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
+ return indexAST(AST.getASTContext(), AST.getPreprocessorPtr()).first;
}
std::unique_ptr TestTU::index() const {
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -74,6 +74,16 @@
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
}
+MATCHER(OccurrenceRange, "") {
+ const clang::clangd::SymbolOccurrence& Pos = testing::get<0>(arg);
+ const clang::clangd::Range& Range = testing::get<1>(arg);
+ return std::tie(Pos.Location.Start.Line,
+ Pos.Location.Start.Column,
+ Pos.Location.End.Line,
+ Pos.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+ Range.end.character);
+}
namespace clang {
namespace clangd {
@@ -237,6 +247,7 @@
llvm::MemoryBuffer::getMemBuffer(MainCode));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
+SymbolOccurrences = Factory->Collector->takeOccurrences();
return true;
}
@@ -247,6 +258,7 @@
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
+ SymbolOccurrenceSlab SymbolOccurrences;
SymbolCollector::Options CollectorOpts;
std::unique_ptr PragmaHandler;
};
@@ -413,6 +425,67 @@
));
}
+TEST_F(SymbolCollectorTest, Occurrences) {
+ Annotations Header(R"(
+ class $foo[[Foo]] {
+ public:
+$foo[[Foo]]() {}
+$foo[[Foo]](int);
+ };
+ class $bar[[Bar]];
+ void $func[[func]]();
+ )");
+ Annotations Main(R"(
+ class $bar[[Bar]] {};
+
+ void $func[[func]]();
+
+ void fff() {
+$foo[[Foo]] foo;
+$bar[[Bar]] bar;
+$func[[func]]();
+int abc = 0;
+$foo[[Foo]] foo2 = abc;
+ }
+ )");
+ Annotations SymbolsOnlyInMainCode(R"(
+ int a;
+ void b() {}
+ static const int c = 0;
+ class d {};
+ )");
+ CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
+ SymbolOccurrenceKind::Definition |
+ SymbolOccurrenceKind::Reference;
+ runSymbolCollector(Header.code(),
+ (Main.code() + SymbolsOnlyInMainCode.code()).str());
+ auto HeaderSymbols = TestTU::withHeaderCode(Header.code()).headerSymbols();
+
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(Symbols, "Foo").ID),
+ testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("foo")));
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(Symbols, "Bar").ID),
+ testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("bar")));
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(Symbols, "func").ID),
+ testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("func")));
+
+ // Retrieve IDs for symbols *only* in the main file, and verify these symbols
+ // are not collected.
+ auto MainSymbols =
+ TestTU::withHeaderCode(SymbolsOnlyInMainCode.code()).headerSymbols();
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(MainSymbols, "a").ID),
+ testing::IsEmpty());
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(MainSymbols, "b").ID),
+ testing::IsEmpty());
+ EXPECT_THAT(
+ SymbolOccurren
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein updated this revision to Diff 163066.
hokein added a comment.
Minor cleanup.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
clangd/index/Merge.h
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -38,6 +38,16 @@
return TU;
}
+ static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+llvm::StringRef Filename = "") {
+TestTU TU;
+TU.HeaderCode = HeaderCode;
+TU.Code = Code;
+if (!Filename.empty())
+ TU.Filename = Filename;
+return TU;
+ }
+
// The code to be compiled.
std::string Code;
std::string Filename = "TestTU.cpp";
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -45,7 +45,7 @@
SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
- return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
+ return indexAST(AST.getASTContext(), AST.getPreprocessorPtr()).first;
}
std::unique_ptr TestTU::index() const {
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -74,6 +74,16 @@
MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
}
+MATCHER(OccurrenceRange, "") {
+ const clang::clangd::SymbolOccurrence& Pos = testing::get<0>(arg);
+ const clang::clangd::Range& Range = testing::get<1>(arg);
+ return std::tie(Pos.Location.Start.Line,
+ Pos.Location.Start.Column,
+ Pos.Location.End.Line,
+ Pos.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+ Range.end.character);
+}
namespace clang {
namespace clangd {
@@ -237,6 +247,7 @@
llvm::MemoryBuffer::getMemBuffer(MainCode));
Invocation.run();
Symbols = Factory->Collector->takeSymbols();
+SymbolOccurrences = Factory->Collector->takeOccurrences();
return true;
}
@@ -247,6 +258,7 @@
std::string TestFileName;
std::string TestFileURI;
SymbolSlab Symbols;
+ SymbolOccurrenceSlab SymbolOccurrences;
SymbolCollector::Options CollectorOpts;
std::unique_ptr PragmaHandler;
};
@@ -413,6 +425,67 @@
));
}
+TEST_F(SymbolCollectorTest, Occurrences) {
+ Annotations Header(R"(
+ class $foo[[Foo]] {
+ public:
+$foo[[Foo]]() {}
+$foo[[Foo]](int);
+ };
+ class $bar[[Bar]];
+ void $func[[func]]();
+ )");
+ Annotations Main(R"(
+ class $bar[[Bar]] {};
+
+ void $func[[func]]();
+
+ void fff() {
+$foo[[Foo]] foo;
+$bar[[Bar]] bar;
+$func[[func]]();
+int abc = 0;
+$foo[[Foo]] foo2 = abc;
+ }
+ )");
+ Annotations SymbolsOnlyInMainCode(R"(
+ int a;
+ void b() {}
+ static const int c = 0;
+ class d {};
+ )");
+ CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
+ SymbolOccurrenceKind::Definition |
+ SymbolOccurrenceKind::Reference;
+ runSymbolCollector(Header.code(),
+ (Main.code() + SymbolsOnlyInMainCode.code()).str());
+ auto HeaderSymbols = TestTU::withHeaderCode(Header.code()).headerSymbols();
+
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(Symbols, "Foo").ID),
+ testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("foo")));
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(Symbols, "Bar").ID),
+ testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("bar")));
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(Symbols, "func").ID),
+ testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("func")));
+
+ // Retrieve IDs for symbols *only* in the main file, and verify these symbols
+ // are not collected.
+ auto MainSymbols =
+ TestTU::withHeaderCode(SymbolsOnlyInMainCode.code()).headerSymbols();
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(MainSymbols, "a").ID),
+ testing::IsEmpty());
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(MainSymbols, "b").ID),
+ testing::IsEmpty());
+ EXPECT_THAT(
+ SymbolOccurrences.find(findSymbol(MainSymbols, "c").ID),
+ testing::IsEmpty());
+}
+
TEST_F(Sym
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein added a comment.
Thanks for the comments.
Comment at: clangd/index/FileIndex.cpp:48
+ if (TopLevelDecls) { // index main AST, set occurrence flag.
+CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
ilya-biryukov wrote:
> There is an implicit assumption (`TopLevelDecls != null` iff indexing the
> main file AST in dynamic index) that does not seem quite obvious.
> Maybe add two overloads (for indexing the main AST and for indexing the
> preamble AST) or add an extra parameter to be explicit more about those
> choices?
This is a hacky way to detect whether we are indexing main AST or preamble AST
-- because the current `FileIndex::update(PathRef Path, ...)` interface doesn't
contain this information, and we use it both for `PreambleAST` and `MainAST`
indexing in the client side (`ParsingCallback`).
I think we might want two dedicated methods (`updateMainAST`, and
`updatePreambleAST`) in `FileSymbol`, and I think we should address it in a
follow-up patch. WDYT?
Comment at: clangd/index/FileIndex.h:50
+ std::vector> allOccurrenceSlabs()
const;
+
sammccall wrote:
> sammccall wrote:
> > This return type seems inconsistent with the design of this class.
> > The purpose of the `shared_ptr>` in `allSymbols` is to
> > avoid exposing the concrete storage.
> > The analogue would I guess be `shared_ptr > vector>>`.
> >
> > The cost of creating that does seem a little gross though.
> allOccurrences
> (name should reflect the semantics, not the type)
> The cost of creating that does seem a little gross though.
Previously I tried to avoid the cost of iterating/copying all occurrences.
After some investigations, it seems that the number of symbol occurrences is
small (~500 for `SemaDecl.cpp`, < 100 for `CodeComplete.cpp`). The cost should
not be too expensive.
Comment at: clangd/index/FileIndex.h:100
/// level decls obtained from \p AST are indexed.
-SymbolSlab
+std::pair
indexAST(ASTContext &AST, std::shared_ptr PP,
ilya-biryukov wrote:
> Maybe use a struct to allow named fields? Would mean a tiny amount of extra
> code in `indexAST`, but should make the client code slightly nicer.
I think the type of `pair` explicitly express its meaning well enough? And this
function is exposed for unittests, I'm happy to do the change if you insist.
Comment at: clangd/index/MemIndex.cpp:36
auto Idx = llvm::make_unique();
- Idx->build(getSymbolsFromSlab(std::move(Slab)));
+ Idx->build(getSymbolsFromSlab(std::move(Slab)), {});
return std::move(Idx);
sammccall wrote:
> we should find a way to avoid having functions implicitly zero out parts of
> the index like this!
Made it explicit.
Comment at: clangd/index/Merge.cpp:95
+Static->findOccurrences(Req, [&](const SymbolOccurrence& O) {
+ if (llvm::is_contained(SeenFiles, O.Location.FileURI))
+return;
ilya-biryukov wrote:
> `llvm::is_contained` seems to be linear time, maybe replace with hash-table
> lookup.
> Or am I missing something?
Good catch, I thought it had a trait for `Map`, `Set`, but it turns out that
`is_contained` is only a `std::find` wrapper :(
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
sammccall added a comment.
In https://reviews.llvm.org/D51279#1214268, @ilya-biryukov wrote:
> Just noticed I'm not on the reviewers list, sorry for chiming in without
> invitation. Was really excited about the change :-)
fixed :-)
Comment at: clangd/index/FileIndex.cpp:58
- return Collector.takeSymbols();
+ vlog("index for AST: ");
+ auto Syms = Collector.takeSymbols();
combine into one log statement
Comment at: clangd/index/FileIndex.h:50
+ std::vector> allOccurrenceSlabs()
const;
+
This return type seems inconsistent with the design of this class.
The purpose of the `shared_ptr>` in `allSymbols` is to
avoid exposing the concrete storage.
The analogue would I guess be `shared_ptr>>`.
The cost of creating that does seem a little gross though.
Comment at: clangd/index/FileIndex.h:50
+ std::vector> allOccurrenceSlabs()
const;
+
sammccall wrote:
> This return type seems inconsistent with the design of this class.
> The purpose of the `shared_ptr>` in `allSymbols` is to
> avoid exposing the concrete storage.
> The analogue would I guess be `shared_ptr vector>>`.
>
> The cost of creating that does seem a little gross though.
allOccurrences
(name should reflect the semantics, not the type)
Comment at: clangd/index/FileIndex.h:57
llvm::StringMap> FileToSlabs;
+ /// \breif Stores the latest occurrence slabs for all active files.
+ llvm::StringMap> FileToOccurrenceSlabs;
nit: no need for brief unless the "first sentence" heuristic fails. (also, it's
misspelled!)
Comment at: clangd/index/FileIndex.h:95
/// Retrieves namespace and class level symbols in \p AST.
/// Exposed to assist in unit tests.
update doc
Comment at: clangd/index/MemIndex.cpp:36
auto Idx = llvm::make_unique();
- Idx->build(getSymbolsFromSlab(std::move(Slab)));
+ Idx->build(getSymbolsFromSlab(std::move(Slab)), {});
return std::move(Idx);
we should find a way to avoid having functions implicitly zero out parts of the
index like this!
Comment at: clangd/index/MemIndex.cpp:86
const OccurrencesRequest &Req,
llvm::function_ref Callback) const {
+ for (const auto &Slab : OccurrenceSlabs) {
This assumes there is no duplication across occurrences, but gives no mechanism
through which deduplication could occur (because we're coupled to the
underlying storage).
If FileIndex provides the DenseMap> then it
could just refrain from providing duplicate pointers.
Comment at: clangd/index/MemIndex.h:27
+ build(std::shared_ptr> Symbols,
+std::vector> OccurrenceSlabs =
{});
ilya-biryukov wrote:
> Maybe remove the default parameter? Making the callers specify the
> occurrences explicitly could make their code more straight-forward to follow.
(type and name: same comment as on FileIndex)
Comment at: clangd/index/Merge.cpp:84
Callback) const override {
-log("findOccurrences is not implemented.");
+// Store all seen files by dynamic index.
+llvm::DenseSet SeenFiles;
I'm tempted to say remove this heuristic for now (as you say, we should solve
this thoroughly), but the artifacts will be severe, so it's probably correct to
include it.
But this comment doesn't explain the heuristic, it just echoes the code.
I'd suggest something like:
```// We don't want duplicate occurrences from the static/dynamic indexes,
// and we can't reliably deduplicate because offsets may differ slightly.
// We consider the dynamic index authoritative and report all its occurrences,
// and only report static index occurrences from other files.
// FIXME: this heuristic fails if the dynamic index contains a file, but all
// occurrences were removed (we will report stale ones from static).
// Ultimately we should explicitly check which index has the file instead.```
Comment at: clangd/index/Merge.cpp:85
+// Store all seen files by dynamic index.
+llvm::DenseSet SeenFiles;
+// Emit all dynamic occurrences, and static occurrences that are not
SeenFiles -> DynamicIndexFiles?
Comment at: clangd/index/Merge.cpp:88
+// in seen files.
+// FIXME: If a file has been updated, and there are no occurrences indexed
+// in dynamic index, stale results are still returned (from static index).
hokein wrote:
> ilya-biryukov wrote:
> > Do we want to fix it before checking this in? As discussed offline, this
> > seems to be a limitation that could affect the design (FileOccurrences vs
> > SymbolOccurrence in the symbol interface). In case we want to avoid
> > refactoring the interfaces later.
> Yes, I re-thought it afterwards. We could solve this issue for
> `findOccurrences` by redefining
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein added a comment. In https://reviews.llvm.org/D51279#1214268, @ilya-biryukov wrote: > Just noticed I'm not on the reviewers list, sorry for chiming in without > invitation. Was really excited about the change :-) Comments are always welcome :) Comment at: clangd/index/Merge.cpp:88 +// in seen files. +// FIXME: If a file has been updated, and there are no occurrences indexed +// in dynamic index, stale results are still returned (from static index). ilya-biryukov wrote: > Do we want to fix it before checking this in? As discussed offline, this > seems to be a limitation that could affect the design (FileOccurrences vs > SymbolOccurrence in the symbol interface). In case we want to avoid > refactoring the interfaces later. Yes, I re-thought it afterwards. We could solve this issue for `findOccurrences` by redefining the interface. Unfortunately, we have the same issue for `fuzzyFind`, `lookup` too -- we still return the stale symbols from static index if the symbol was deleted in dirty files. I think we should address this issue thoroughly and separately -- we probably want to add a method in dynamic index for query whether a file is in. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
ilya-biryukov added a comment. Just noticed I'm not on the reviewers list, sorry for chiming in without invitation. Was really excited about the change :-) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
ilya-biryukov added a comment.
The memory usage looks good. Some NITs and a major consideration wrt to the
implementation of merging occurences from dynamic and static index.
Comment at: clangd/index/FileIndex.cpp:48
+ if (TopLevelDecls) { // index main AST, set occurrence flag.
+CollectorOpts.OccurrenceFilter = SymbolOccurrenceKind::Declaration |
There is an implicit assumption (`TopLevelDecls != null` iff indexing the main
file AST in dynamic index) that does not seem quite obvious.
Maybe add two overloads (for indexing the main AST and for indexing the
preamble AST) or add an extra parameter to be explicit more about those choices?
Comment at: clangd/index/FileIndex.cpp:74
+ if (!Slab) {
FileToSlabs.erase(Path);
+ } else {
NIT: maybe remove braces around single-statement branches?
Comment at: clangd/index/FileIndex.cpp:126
auto Slab = llvm::make_unique();
-*Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes);
-FSymbols.update(Path, std::move(Slab));
+auto IndexResults = indexAST(*AST, PP, TopLevelDecls, URISchemes);
+*Slab = std::move(IndexResults.first);
Why not `std::tie(*Slab, *OccurenceSlab) = indexAST(...)`?
I assume this should work and give the optimal performance
Comment at: clangd/index/FileIndex.h:45
+ void update(PathRef Path, std::unique_ptr Slab,
+ std::unique_ptr Occurrences = nullptr);
Maybe avoid default arguments? Having clients pass nullptr explicitly seems
like the right thing to do
Comment at: clangd/index/FileIndex.h:100
/// level decls obtained from \p AST are indexed.
-SymbolSlab
+std::pair
indexAST(ASTContext &AST, std::shared_ptr PP,
Maybe use a struct to allow named fields? Would mean a tiny amount of extra
code in `indexAST`, but should make the client code slightly nicer.
Comment at: clangd/index/MemIndex.h:27
+ build(std::shared_ptr> Symbols,
+std::vector> OccurrenceSlabs =
{});
Maybe remove the default parameter? Making the callers specify the occurrences
explicitly could make their code more straight-forward to follow.
Comment at: clangd/index/Merge.cpp:88
+// in seen files.
+// FIXME: If a file has been updated, and there are no occurrences indexed
+// in dynamic index, stale results are still returned (from static index).
Do we want to fix it before checking this in? As discussed offline, this seems
to be a limitation that could affect the design (FileOccurrences vs
SymbolOccurrence in the symbol interface). In case we want to avoid refactoring
the interfaces later.
Comment at: clangd/index/Merge.cpp:95
+Static->findOccurrences(Req, [&](const SymbolOccurrence& O) {
+ if (llvm::is_contained(SeenFiles, O.Location.FileURI))
+return;
`llvm::is_contained` seems to be linear time, maybe replace with hash-table
lookup.
Or am I missing something?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein added a comment. Some numbers of memory usage from running this on my machine: | File | Preamble AST | Main AST | | SemaDecl.cpp | 14.5MB | 108.1KB (symbols) + 16.5KB (occurrences) | | CodeComplete.cpp | 15.4MB | 53.9KB (symbols) + 7.3KB (occurrences) | | The memory usage of `occurrences` is relatively small, I think it is fine to enable it by default. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.
hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric,
ilya-biryukov.
Implement the interface in
- FileIndex
- MemIndex
- MergeIndex
Depends on https://reviews.llvm.org/D50385.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51279
Files:
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/MemIndex.h
clangd/index/Merge.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -38,6 +38,16 @@
return TU;
}
+ static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+llvm::StringRef Filename = "") {
+TestTU TU;
+TU.HeaderCode = HeaderCode;
+TU.Code = Code;
+if (!Filename.empty())
+ TU.Filename = Filename;
+return TU;
+ }
+
// The code to be compiled.
std::string Code;
std::string Filename = "TestTU.cpp";
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -45,7 +45,7 @@
SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
- return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
+ return indexAST(AST.getASTContext(), AST.getPreprocessorPtr()).first;
}
std::unique_ptr TestTU::index() const {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -8,20 +8,33 @@
//===--===//
#include "TestIndex.h"
+#include "TestTU.h"
+#include "Annotations.h"
#include "index/Index.h"
#include "index/MemIndex.h"
#include "index/Merge.h"
+#include "index/FileIndex.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
using testing::Pointee;
using testing::UnorderedElementsAre;
+using testing::AllOf;
namespace clang {
namespace clangd {
namespace {
MATCHER_P(Named, N, "") { return arg.Name == N; }
+MATCHER_P(OccurrenceRange, Range, "") {
+ return std::tie(arg.Location.Start.Line,
+ arg.Location.Start.Column,
+ arg.Location.End.Line,
+ arg.Location.End.Column) ==
+ std::tie(Range.start.line, Range.start.character, Range.end.line,
+ Range.end.character);
+}
+MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
TEST(SymbolSlab, FindAndIterate) {
SymbolSlab::Builder B;
@@ -243,6 +256,55 @@
EXPECT_EQ(M.Name, "right");
}
+TEST(MergeIndexTest, FindOccurrences) {
+ FileIndex Dyn({"unittest"});
+ FileIndex StaticIndex({"unittest"});
+ auto MergedIndex = mergeIndex(&Dyn, &StaticIndex);
+
+ const char* HeaderCode = "class Foo;";
+ auto HeaderSymbols = TestTU::withHeaderCode("class Foo;").headerSymbols();
+ auto Foo = findSymbol(HeaderSymbols, "Foo");
+
+ // Build dynamic index.
+ Annotations Test1Code(R"(class $Foo[[Foo]];)");
+ auto AST =
+ TestTU::withAllCode(HeaderCode, Test1Code.code(), "test.cc").build();
+ Dyn.update("test.cc", &AST.getASTContext(), AST.getPreprocessorPtr(),
+ AST.getLocalTopLevelDecls());
+
+ // Build static index.
+ auto StaticAST =
+ TestTU::withAllCode(HeaderCode, "// static\nclass Foo {};", "test.cc")
+ .build();
+ // Add stale occurrences for test.cc.
+ StaticIndex.update("test.cc", &StaticAST.getASTContext(),
+ StaticAST.getPreprocessorPtr(),
+ StaticAST.getLocalTopLevelDecls());
+
+ // Add occcurrences for test2.cc
+ Annotations Test2Code(R"(class $Foo[[Foo]] {};)");
+ StaticAST =
+ TestTU::withAllCode(HeaderCode, Test2Code.code(), "test2.cc").build();
+ StaticIndex.update("test2.cc", &StaticAST.getASTContext(),
+ StaticAST.getPreprocessorPtr(),
+ StaticAST.getLocalTopLevelDecls());
+
+ OccurrencesRequest Request;
+ Request.IDs = {Foo.ID};
+ Request.Filter = SymbolOccurrenceKind::Declaration |
+ SymbolOccurrenceKind::Definition |
+ SymbolOccurrenceKind::Reference;
+ std::vector Results;
+ MergedIndex->findOccurrences(
+ Request, [&](const SymbolOccurrence &O) { Results.push_back(O); });
+
+ EXPECT_THAT(Results,
+ UnorderedElementsAre(AllOf(OccurrenceRange(Test1Code.range("Foo")),
+ FileURI("unittest:///test.cc")),
+ AllOf(OccurrenceRange(Test2Code.range("Foo")),
+ FileURI("unittest:///test2.cc";
+}
+
} // namespace
} // namespace clangd
} // namesp
