[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Looks good in general.



================
Comment at: clangd/ClangdLSPServer.h:117
+        llvm::Optional<Path> CompileCommandsDir,
+        std::function<void(llvm::StringRef,
+                           const tooling::CompilationDatabase &)>);
----------------
please document what the callback is for and how often it's called.


================
Comment at: clangd/index/Background.cpp:63
+                                 const tooling::CompilationDatabase &CDB) {
+  // TODO: this function may be slow. Perhaps enqueue a task to re-read the CDB
+  // from disk and enqueue the commands asynchronously?
----------------
Maybe add a trace?


================
Comment at: clangd/index/Background.cpp:98
+  }
+  llvm::sys::path::native(AbsolutePath);
+
----------------
Why is this needed?


================
Comment at: clangd/index/Background.cpp:163
+  vlog("Rebuilding automatic index");
+  reset(IndexedSymbols.buildMemIndex());
+  return Error::success();
----------------
Add a FIXME to use Dex?


================
Comment at: clangd/index/Background.h:1
+//===--- Background.h - Build an index in a background thread ----*- C++-*-===//
+//
----------------
Is it possible to add test for this?


================
Comment at: clangd/index/Background.h:28
+// all commands in a compilation database. Indexing happens in the background.
+// TODO: it should also persist its state on disk for fast start.
+class BackgroundIndex : public SwapIndex {
----------------
s/TODO/FIXME/?


================
Comment at: clangd/index/Background.h:39
+  // The indexing happens in a background thread, so after enqueueing files
+  // for indexing their symbols will be available sometime later.
+  void enqueueAll(llvm::StringRef Directory,
----------------
Maybe add a FIXME for files not in CDB e.g. headers? 


================
Comment at: clangd/tool/ClangdMain.cpp:170
 
+static llvm::cl::opt<bool> AutoIndex(
+    "auto-index",
----------------
Should we ignore index-file/auto-index if one is set?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53032