[PATCH] D41668: [clangd] Add static index for the global code completion.

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 3 00:21:14 PST 2018

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 {

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.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list