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

[Eric Liu via Phabricator via cfe-commits]

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