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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments as done.
sammccall added inline comments.


================
Comment at: clangd/index/Background.cpp:98
+  }
+  llvm::sys::path::native(AbsolutePath);
+
----------------
ioeric wrote:
> Why is this needed?
Removed. I copied it from the related code in JSONCompilationDatabase, but I'm not sure what the right behavior is, as JSONCompilationDatabase doesn't specify the format of paths.


================
Comment at: clangd/index/Background.cpp:163
+  vlog("Rebuilding automatic index");
+  reset(IndexedSymbols.buildMemIndex());
+  return Error::success();
----------------
ioeric wrote:
> Add a FIXME to use Dex?
Added it to the existing TODO. They're coupled: dex is expensive to build, so rebuilding it after every file wouldnt' make sense.


================
Comment at: clangd/index/Background.h:1
+//===--- Background.h - Build an index in a background thread ----*- C++-*-===//
+//
----------------
ioeric wrote:
> Is it possible to add test for this?
Added a test that indexes a file, waits for the queue to be empty, and queries.

That's pretty basic, do you have suggestions for things you'd like to see tested?


================
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,
----------------
ioeric wrote:
> Maybe add a FIXME for files not in CDB e.g. headers? 
There's nothing missing for headers at this point.
Headers will already be indexed (as part of any TU that includes them).
Later, we'll partition the outputs by file, but these headers will still be indexed.
When it comes time to watch files on disk for changes, then we'll need to consider headers specially.

Indexing headers that are not included by any file is out of scope (it's not clear this is a good idea, e.g. consider files under test/ in llvm).


================
Comment at: clangd/tool/ClangdMain.cpp:170
 
+static llvm::cl::opt<bool> AutoIndex(
+    "auto-index",
----------------
ioeric wrote:
> Should we ignore index-file/auto-index if one is set?
Since this is hidden/experimental, I think it's not worth adding explicit code so they interact nicely, I just documented the behavior instead instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53032