Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -206,7 +206,7 @@ /// Returns the active dynamic index if one was built. /// This can be useful for testing, debugging, or observing memory usage. - const SymbolIndex *dynamicIndex() const; + const SymbolIndex *dynamicIndex() const { return DynamicIdx.get(); } // Blocks the main thread until the server is idle. Only for use in tests. // Returns false if the timeout expires. @@ -244,7 +244,7 @@ // If present, an index of symbols in open files. Read via *Index. std::unique_ptr DynamicIdx; // If present, storage for the merged static/dynamic index. Read via *Index. - std::unique_ptr MergedIndex; + std::unique_ptr MergedIdx; // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex) llvm::StringMap> Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -117,20 +117,17 @@ : nullptr, Opts.UpdateDebounce, Opts.RetentionPolicy) { if (DynamicIdx && Opts.StaticIndex) { - MergedIndex = mergeIndex(&DynamicIdx->index(), Opts.StaticIndex); - Index = MergedIndex.get(); + MergedIdx = + llvm::make_unique(DynamicIdx.get(), Opts.StaticIndex); + Index = MergedIdx.get(); } else if (DynamicIdx) - Index = &DynamicIdx->index(); + Index = DynamicIdx.get(); else if (Opts.StaticIndex) Index = Opts.StaticIndex; else Index = nullptr; } -const SymbolIndex *ClangdServer::dynamicIndex() const { - return DynamicIdx ? &DynamicIdx->index() : nullptr; -} - void ClangdServer::setRootPath(PathRef RootPath) { auto FS = FSProvider.getFileSystem(); auto Status = FS->status(RootPath); Index: clangd/index/FileIndex.h =================================================================== --- clangd/index/FileIndex.h +++ clangd/index/FileIndex.h @@ -19,6 +19,7 @@ #include "ClangdUnit.h" #include "Index.h" #include "MemIndex.h" +#include "Merge.h" #include "clang/Lex/Preprocessor.h" #include @@ -59,15 +60,12 @@ /// This manages symbols from files and an in-memory index on all symbols. /// FIXME: Expose an interface to remove files that are closed. -class FileIndex { +class FileIndex : public MergedIndex { public: /// If URISchemes is empty, the default schemes in SymbolCollector will be /// used. FileIndex(std::vector URISchemes = {}); - // Presents a merged view of the supplied main-file and preamble ASTs. - const SymbolIndex &index() const { return *MergedIndex; } - /// Update preamble symbols of file \p Path with all declarations in \p AST /// and macros in \p PP. void updatePreamble(PathRef Path, ASTContext &AST, @@ -102,8 +100,6 @@ // (Note that symbols *only* in the main file are not indexed). FileSymbols MainFileSymbols; SwapIndex MainFileIndex; - - std::unique_ptr MergedIndex; // Merge preamble and main index. }; /// Retrieves symbols and refs of local top level decls in \p AST (i.e. Index: clangd/index/FileIndex.cpp =================================================================== --- clangd/index/FileIndex.cpp +++ clangd/index/FileIndex.cpp @@ -152,10 +152,10 @@ } FileIndex::FileIndex(std::vector URISchemes) - : URISchemes(std::move(URISchemes)), + : MergedIndex(&MainFileIndex, &PreambleIndex), + URISchemes(std::move(URISchemes)), PreambleIndex(PreambleSymbols.buildMemIndex()), - MainFileIndex(MainFileSymbols.buildMemIndex()), - MergedIndex(mergeIndex(&MainFileIndex, &PreambleIndex)) {} + MainFileIndex(MainFileSymbols.buildMemIndex()) {} void FileIndex::updatePreamble(PathRef Path, ASTContext &AST, std::shared_ptr PP) { Index: clangd/index/Merge.h =================================================================== --- clangd/index/Merge.h +++ clangd/index/Merge.h @@ -20,7 +20,7 @@ // Returned symbol may contain data owned by either source. Symbol mergeSymbol(const Symbol &L, const Symbol &R); -// mergeIndex returns a composite index based on two provided Indexes: +// MergedIndex is a composite index based on two provided Indexes: // - the Dynamic index covers few files, but is relatively up-to-date. // - the Static index covers a bigger set of files, but is relatively stale. // The returned index attempts to combine results, and avoid duplicates. @@ -28,8 +28,25 @@ // FIXME: We don't have a mechanism in Index to track deleted symbols and // refs in dirty files, so the merged index may return stale symbols // and refs from Static index. -std::unique_ptr mergeIndex(const SymbolIndex *Dynamic, - const SymbolIndex *Static); +class MergedIndex : public SymbolIndex { + const SymbolIndex *Dynamic, *Static; + +public: + // The constructor does not access the symbols. + // It's safe to inherit from this class and pass pointers to derived members. + MergedIndex(const SymbolIndex *Dynamic, const SymbolIndex *Static) + : Dynamic(Dynamic), Static(Static) {} + + bool fuzzyFind(const FuzzyFindRequest &, + llvm::function_ref) const override; + void lookup(const LookupRequest &, + llvm::function_ref) const override; + void refs(const RefsRequest &, + llvm::function_ref) const override; + size_t estimateMemoryUsage() const override { + return Dynamic->estimateMemoryUsage() + Static->estimateMemoryUsage(); + } +}; } // namespace clangd } // namespace clang Index: clangd/index/Merge.cpp =================================================================== --- clangd/index/Merge.cpp +++ clangd/index/Merge.cpp @@ -17,111 +17,96 @@ namespace clang { namespace clangd { -namespace { using namespace llvm; -class MergedIndex : public SymbolIndex { - public: - MergedIndex(const SymbolIndex *Dynamic, const SymbolIndex *Static) - : Dynamic(Dynamic), Static(Static) {} - - // FIXME: Deleted symbols in dirty files are still returned (from Static). - // To identify these eliminate these, we should: - // - find the generating file from each Symbol which is Static-only - // - ask Dynamic if it has that file (needs new SymbolIndex method) - // - if so, drop the Symbol. - bool fuzzyFind(const FuzzyFindRequest &Req, - function_ref Callback) const override { - // We can't step through both sources in parallel. So: - // 1) query all dynamic symbols, slurping results into a slab - // 2) query the static symbols, for each one: - // a) if it's not in the dynamic slab, yield it directly - // b) if it's in the dynamic slab, merge it and yield the result - // 3) now yield all the dynamic symbols we haven't processed. - trace::Span Tracer("MergedIndex fuzzyFind"); - bool More = false; // We'll be incomplete if either source was. - SymbolSlab::Builder DynB; - unsigned DynamicCount = 0; - unsigned StaticCount = 0; - unsigned MergedCount = 0; - More |= Dynamic->fuzzyFind(Req, [&](const Symbol &S) { - ++DynamicCount; - DynB.insert(S); - }); - SymbolSlab Dyn = std::move(DynB).build(); - - DenseSet SeenDynamicSymbols; - More |= Static->fuzzyFind(Req, [&](const Symbol &S) { - auto DynS = Dyn.find(S.ID); - ++StaticCount; - if (DynS == Dyn.end()) - return Callback(S); - ++MergedCount; - SeenDynamicSymbols.insert(S.ID); - Callback(mergeSymbol(*DynS, S)); - }); - SPAN_ATTACH(Tracer, "dynamic", DynamicCount); - SPAN_ATTACH(Tracer, "static", StaticCount); - SPAN_ATTACH(Tracer, "merged", MergedCount); - for (const Symbol &S : Dyn) - if (!SeenDynamicSymbols.count(S.ID)) - Callback(S); - return More; - } +// FIXME: Deleted symbols in dirty files are still returned (from Static). +// To identify these eliminate these, we should: +// - find the generating file from each Symbol which is Static-only +// - ask Dynamic if it has that file (needs new SymbolIndex method) +// - if so, drop the Symbol. +bool MergedIndex::fuzzyFind(const FuzzyFindRequest &Req, + function_ref Callback) const { + // We can't step through both sources in parallel. So: + // 1) query all dynamic symbols, slurping results into a slab + // 2) query the static symbols, for each one: + // a) if it's not in the dynamic slab, yield it directly + // b) if it's in the dynamic slab, merge it and yield the result + // 3) now yield all the dynamic symbols we haven't processed. + trace::Span Tracer("MergedIndex fuzzyFind"); + bool More = false; // We'll be incomplete if either source was. + SymbolSlab::Builder DynB; + unsigned DynamicCount = 0; + unsigned StaticCount = 0; + unsigned MergedCount = 0; + More |= Dynamic->fuzzyFind(Req, [&](const Symbol &S) { + ++DynamicCount; + DynB.insert(S); + }); + SymbolSlab Dyn = std::move(DynB).build(); + + DenseSet SeenDynamicSymbols; + More |= Static->fuzzyFind(Req, [&](const Symbol &S) { + auto DynS = Dyn.find(S.ID); + ++StaticCount; + if (DynS == Dyn.end()) + return Callback(S); + ++MergedCount; + SeenDynamicSymbols.insert(S.ID); + Callback(mergeSymbol(*DynS, S)); + }); + SPAN_ATTACH(Tracer, "dynamic", DynamicCount); + SPAN_ATTACH(Tracer, "static", StaticCount); + SPAN_ATTACH(Tracer, "merged", MergedCount); + for (const Symbol &S : Dyn) + if (!SeenDynamicSymbols.count(S.ID)) + Callback(S); + return More; +} - void - lookup(const LookupRequest &Req, - llvm::function_ref Callback) const override { - trace::Span Tracer("MergedIndex lookup"); - SymbolSlab::Builder B; - - Dynamic->lookup(Req, [&](const Symbol &S) { B.insert(S); }); - - auto RemainingIDs = Req.IDs; - Static->lookup(Req, [&](const Symbol &S) { - const Symbol *Sym = B.find(S.ID); - RemainingIDs.erase(S.ID); - if (!Sym) - Callback(S); - else - Callback(mergeSymbol(*Sym, S)); - }); - for (const auto &ID : RemainingIDs) - if (const Symbol *Sym = B.find(ID)) - Callback(*Sym); - } +void MergedIndex::lookup( + const LookupRequest &Req, + llvm::function_ref Callback) const { + trace::Span Tracer("MergedIndex lookup"); + SymbolSlab::Builder B; + + Dynamic->lookup(Req, [&](const Symbol &S) { B.insert(S); }); + + auto RemainingIDs = Req.IDs; + Static->lookup(Req, [&](const Symbol &S) { + const Symbol *Sym = B.find(S.ID); + RemainingIDs.erase(S.ID); + if (!Sym) + Callback(S); + else + Callback(mergeSymbol(*Sym, S)); + }); + for (const auto &ID : RemainingIDs) + if (const Symbol *Sym = B.find(ID)) + Callback(*Sym); +} - void refs(const RefsRequest &Req, - llvm::function_ref Callback) const override { - trace::Span Tracer("MergedIndex refs"); - // We don't want duplicated refs from the static/dynamic indexes, - // and we can't reliably duplicate them because offsets may differ slightly. - // We consider the dynamic index authoritative and report all its refs, - // and only report static index refs from other files. - // - // FIXME: The heuristic fails if the dynamic index contains a file, but all - // refs were removed (we will report stale ones from the static index). - // Ultimately we should explicit check which index has the file instead. - llvm::StringSet<> DynamicIndexFileURIs; - Dynamic->refs(Req, [&](const Ref &O) { - DynamicIndexFileURIs.insert(O.Location.FileURI); +void MergedIndex::refs(const RefsRequest &Req, + llvm::function_ref Callback) const { + trace::Span Tracer("MergedIndex refs"); + // We don't want duplicated refs from the static/dynamic indexes, + // and we can't reliably duplicate them because offsets may differ slightly. + // We consider the dynamic index authoritative and report all its refs, + // and only report static index refs from other files. + // + // FIXME: The heuristic fails if the dynamic index contains a file, but all + // refs were removed (we will report stale ones from the static index). + // Ultimately we should explicit check which index has the file instead. + llvm::StringSet<> DynamicIndexFileURIs; + Dynamic->refs(Req, [&](const Ref &O) { + DynamicIndexFileURIs.insert(O.Location.FileURI); + Callback(O); + }); + Static->refs(Req, [&](const Ref &O) { + if (!DynamicIndexFileURIs.count(O.Location.FileURI)) Callback(O); - }); - Static->refs(Req, [&](const Ref &O) { - if (!DynamicIndexFileURIs.count(O.Location.FileURI)) - Callback(O); - }); - } - - size_t estimateMemoryUsage() const override { - return Dynamic->estimateMemoryUsage() + Static->estimateMemoryUsage(); - } - -private: - const SymbolIndex *Dynamic, *Static; -}; -} // namespace + }); +} Symbol mergeSymbol(const Symbol &L, const Symbol &R) { assert(L.ID == R.ID); @@ -169,10 +154,5 @@ return S; } -std::unique_ptr mergeIndex(const SymbolIndex *Dynamic, - const SymbolIndex *Static) { - return llvm::make_unique(Dynamic, Static); -} - } // namespace clangd } // namespace clang Index: unittests/clangd/FileIndexTests.cpp =================================================================== --- unittests/clangd/FileIndexTests.cpp +++ unittests/clangd/FileIndexTests.cpp @@ -120,10 +120,10 @@ EXPECT_THAT(getRefs(*Symbols, ID), RefsAre({FileURI("f1.cc")})); } -std::vector match(const FileIndex &I, +std::vector match(const SymbolIndex &I, const FuzzyFindRequest &Req) { std::vector Matches; - I.index().fuzzyFind(Req, [&](const Symbol &Sym) { + I.fuzzyFind(Req, [&](const Symbol &Sym) { Matches.push_back((Sym.Scope + Sym.Name).str()); }); return Matches; @@ -147,7 +147,7 @@ FuzzyFindRequest Req; Req.Query = ""; bool SeenSymbol = false; - M.index().fuzzyFind(Req, [&](const Symbol &Sym) { + M.fuzzyFind(Req, [&](const Symbol &Sym) { EXPECT_EQ(Sym.CanonicalDeclaration.FileURI, "unittest:///f.h"); SeenSymbol = true; }); @@ -201,7 +201,7 @@ FuzzyFindRequest Req; Req.Query = ""; bool SeenSymbol = false; - M.index().fuzzyFind(Req, [&](const Symbol &Sym) { + M.fuzzyFind(Req, [&](const Symbol &Sym) { EXPECT_TRUE(Sym.IncludeHeaders.empty()); SeenSymbol = true; }); @@ -225,7 +225,7 @@ Req.Query = ""; bool SeenVector = false; bool SeenMakeVector = false; - M.index().fuzzyFind(Req, [&](const Symbol &Sym) { + M.fuzzyFind(Req, [&](const Symbol &Sym) { if (Sym.Name == "vector") { EXPECT_EQ(Sym.Signature, ""); EXPECT_EQ(Sym.CompletionSnippetSuffix, "<${1:class Ty}>"); @@ -324,7 +324,7 @@ AST = Test2.build(); Index.updateMain(Test2.Filename, AST); - EXPECT_THAT(getRefs(Index.index(), Foo.ID), + EXPECT_THAT(getRefs(Index, Foo.ID), RefsAre({AllOf(RefRange(MainCode.range("foo")), FileURI("unittest:///test.cc")), AllOf(RefRange(MainCode.range("foo")), @@ -338,7 +338,7 @@ FuzzyFindRequest Req; Req.Query = ""; bool SeenSymbol = false; - M.index().fuzzyFind(Req, [&](const Symbol &Sym) { + M.fuzzyFind(Req, [&](const Symbol &Sym) { EXPECT_EQ(Sym.Name, "CLANGD"); EXPECT_EQ(Sym.SymInfo.Kind, index::SymbolKind::Macro); SeenSymbol = true; Index: unittests/clangd/IndexTests.cpp =================================================================== --- unittests/clangd/IndexTests.cpp +++ unittests/clangd/IndexTests.cpp @@ -161,16 +161,16 @@ TEST(MergeIndexTest, Lookup) { auto I = MemIndex::build(generateSymbols({"ns::A", "ns::B"}), RefSlab()), J = MemIndex::build(generateSymbols({"ns::B", "ns::C"}), RefSlab()); - auto M = mergeIndex(I.get(), J.get()); - EXPECT_THAT(lookup(*M, SymbolID("ns::A")), UnorderedElementsAre("ns::A")); - EXPECT_THAT(lookup(*M, SymbolID("ns::B")), UnorderedElementsAre("ns::B")); - EXPECT_THAT(lookup(*M, SymbolID("ns::C")), UnorderedElementsAre("ns::C")); - EXPECT_THAT(lookup(*M, {SymbolID("ns::A"), SymbolID("ns::B")}), + MergedIndex M(I.get(), J.get()); + EXPECT_THAT(lookup(M, SymbolID("ns::A")), UnorderedElementsAre("ns::A")); + EXPECT_THAT(lookup(M, SymbolID("ns::B")), UnorderedElementsAre("ns::B")); + EXPECT_THAT(lookup(M, SymbolID("ns::C")), UnorderedElementsAre("ns::C")); + EXPECT_THAT(lookup(M, {SymbolID("ns::A"), SymbolID("ns::B")}), UnorderedElementsAre("ns::A", "ns::B")); - EXPECT_THAT(lookup(*M, {SymbolID("ns::A"), SymbolID("ns::C")}), + EXPECT_THAT(lookup(M, {SymbolID("ns::A"), SymbolID("ns::C")}), UnorderedElementsAre("ns::A", "ns::C")); - EXPECT_THAT(lookup(*M, SymbolID("ns::D")), UnorderedElementsAre()); - EXPECT_THAT(lookup(*M, {}), UnorderedElementsAre()); + EXPECT_THAT(lookup(M, SymbolID("ns::D")), UnorderedElementsAre()); + EXPECT_THAT(lookup(M, {}), UnorderedElementsAre()); } TEST(MergeIndexTest, FuzzyFind) { @@ -178,7 +178,7 @@ J = MemIndex::build(generateSymbols({"ns::B", "ns::C"}), RefSlab()); FuzzyFindRequest Req; Req.Scopes = {"ns::"}; - EXPECT_THAT(match(*mergeIndex(I.get(), J.get()), Req), + EXPECT_THAT(match(MergedIndex(I.get(), J.get()), Req), UnorderedElementsAre("ns::A", "ns::B", "ns::C")); } @@ -231,7 +231,7 @@ TEST(MergeIndexTest, Refs) { FileIndex Dyn({"unittest"}); FileIndex StaticIndex({"unittest"}); - auto MergedIndex = mergeIndex(&Dyn.index(), &StaticIndex.index()); + MergedIndex Merge(&Dyn, &StaticIndex); const char *HeaderCode = "class Foo;"; auto HeaderSymbols = TestTU::withHeaderCode("class Foo;").headerSymbols(); @@ -266,7 +266,7 @@ RefsRequest Request; Request.IDs = {Foo.ID}; RefSlab::Builder Results; - MergedIndex->refs(Request, [&](const Ref &O) { Results.insert(Foo.ID, O); }); + Merge.refs(Request, [&](const Ref &O) { Results.insert(Foo.ID, O); }); EXPECT_THAT( std::move(Results).build(), Index: unittests/clangd/TestTU.cpp =================================================================== --- unittests/clangd/TestTU.cpp +++ unittests/clangd/TestTU.cpp @@ -48,11 +48,12 @@ return indexHeaderSymbols(AST.getASTContext(), AST.getPreprocessorPtr()); } -// FIXME: This should return a FileIndex with both preamble and main index. std::unique_ptr TestTU::index() const { auto AST = build(); - auto Content = indexMainDecls(AST); - return MemIndex::build(std::move(Content.first), std::move(Content.second)); + auto Idx = llvm::make_unique(); + Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr()); + Idx->updateMain(Filename, AST); + return Idx; } const Symbol &findSymbol(const SymbolSlab &Slab, llvm::StringRef QName) {