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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
