ioeric added inline comments.
================ Comment at: clangd/ClangdLSPServer.h:40 + bool BuildDynamicSymbolIndex, + std::unique_ptr<SymbolIndex> GlobalIdx); ---------------- We are calling this global index and static index in the patch. I think we should be consistent with the naming. Generally, I think this is static index, which might be global index or, say, a set of symbols for test purpose, so I am inclined to static index. ================ Comment at: clangd/CodeComplete.cpp:583 - Items->isIncomplete = !Index.fuzzyFind(Ctx, Req, [&](const Symbol &Sym) { - Items->items.push_back(indexCompletionItem(Sym, Filter, SSInfo)); - }); + // FIXME: figure out a way to merge the symbols from dynamic index and static + // index. ---------------- I would suggest also addressing this `FIXME` in this patch, or at least make it possible to tell from which index source a candidate comes on the client side. Simple concatenation from both indexes would make the results hard to interpret, especially when we don't have a good merging algorithm at this point. ================ Comment at: clangd/CodeComplete.h:72 + /// Static index for project-wide global symbols. It is set by the clangd + /// client. + /// The static index is loaded and built only once when clangd is being ---------------- `clangd client` is a confusing term? Do you mean clangd library users? Also, I think this field is only set internally by clangd in this patch. ================ Comment at: clangd/CodeComplete.h:74 + /// The static index is loaded and built only once when clangd is being + /// launched. + const SymbolIndex *StaticIndex = nullptr; ---------------- This depends on users, so I would drop this line. ================ Comment at: clangd/index/MemIndex.cpp:56 +std::unique_ptr<SymbolIndex> BuildMemIndex(SymbolSlab::Builder Slab) { + struct Snapshot { ---------------- s/Slab/Builder/? Any reason not to pass in a `SymbolSlab` directly? ================ Comment at: clangd/index/MemIndex.h:39 +std::unique_ptr<SymbolIndex> BuildMemIndex(SymbolSlab::Builder Slab); + ---------------- I'd make this a factory method and call it `Build`. ================ Comment at: clangd/tool/ClangdMain.cpp:119 +static llvm::cl::opt<Path> GlobalIndexFile( + "global-index-file", ---------------- Maybe `YamlSymbolFile` or something similar without "global"? Yaml could potentially be used for other static indexes. ================ Comment at: unittests/clangd/CodeCompleteTests.cpp:487 +TEST(CompletionTest, StaticIndex) { + clangd::CodeCompleteOptions Opts; ---------------- We should also have a test where results come from both static index and ast index. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41668 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits