[PATCH] D59605: [clangd] Introduce background-indexer

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I would propose to change the name of the tool.
The "background" part only makes sense in the context of running inside clangd, the index stored on disk is not "background" in any manner.
I'd go with something like `build-clangd-index` (the best option is probably `clangd-indexer`, but it's is already taken and we don't want to confuse the two tools).

Also not sure whether the `BackgroundIndex` is the right abstraction to use here. Conceptually, we want everything from the background index, except the "background" part and the fact that it listens to updates for compile commands (more stuff might get inconsistent in the future, e.g if we start watching for changes to headers, we would probably want to disable this functionality here). I.e. we went with `BackgroundIndex` for code reuse, not because it's the right abstraction for the job. I do think we could come up with something sensible.

That said, the right abstraction is not there now and it may take considerable refactoring to come up with one. And the tool seems to be useful regardless of the underlying implementation, not exactly sure what's the balance we should strike here (time spent vs useful results).

Kadir, what are your thoughts on that matter?



================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:29
+
+static llvm::cl::opt<std::string> CompileCommandsDir(
+    llvm::cl::Positional,
----------------
That's not the accurate name or description, right?
We would actually walk up the directory tree as normal and discover any compilation database up the tree, right?

Maybe rename and document accordingly?


================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:40
+
+  llvm::errs().SetBuffered();
+  clang::clangd::StreamLogger Logger(llvm::errs(), clang::clangd::Logger::Info);
----------------
Why? Maybe add a comment?


================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:47
+  clang::clangd::OverlayCDB CDB(&DirectoryBasedCDB);
+  // FIXME: Maybe add a way to not lower thread priority?
+  clang::clangd::BackgroundIndex BackgroundIdx(
----------------
I think that's actually a hard requirement.
We definitely want to run with normal priority when running as a separate tool.


================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:56
+      // non-interactive tools like this one.
+      24 * 60 * 60 * 1000);
+
----------------
Maybe use `std::numeric_limits<size_t>::max()` here?
Would mean we'll never build the dex index, exactly what we want here.


================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:65
+  llvm::sys::path::append(DummyFile, "dummy.cpp");
+  CDB.getCompileCommand(DummyFile);
+
----------------
We seem to be fighting with the interface we defined here.
If we need to support an operation of re-building the index for the underlying CDB, let's add it explicitly.


================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:68
+  // Wait untill indexing finishes.
+  BackgroundIdx.blockUntilIdleForTest(llvm::None);
+  return 0;
----------------
We can't leave the function called like this, since we now seem to have a legitimate usage outside the test code.

Maybe rename it accordingly?


================
Comment at: clang-tools-extra/clangd/index/Background.h:149
+  // For logging
+  std::atomic<size_t> EnqueuedTUs{0};
+  std::atomic<size_t> IndexedTUs{0};
----------------
This change is not related to the rest of the patch.
Even adding some stats makes sense, could you move into a separate change to keep this change minimal?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59605/new/

https://reviews.llvm.org/D59605