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

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.


================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:56
+      // non-interactive tools like this one.
+      24 * 60 * 60 * 1000);
+  llvm::SmallString<128> DummyFile(CompileCommandsDir);
----------------
Nit: maybe we should give this constant a name? Or maybe create a command line option for this?


================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:60
+  llvm::sys::path::remove_dots(DummyFile, true);
+  llvm::sys::path::append(DummyFile, "dummy.cpp");
+  CDB.getCompileCommand(DummyFile);
----------------
Maybe we should add a comment about why we do this?


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:605
         continue;
+      --UpToDateTUs;
       // FIXME: Currently, we simply schedule indexing on a TU whenever any of
----------------
It's not obvious to me that we don't underflow here given we are using `size_t`. Maybe add an `assert` or some other sanity check?


================
Comment at: clang-tools-extra/clangd/index/Background.h:149
+  // For logging
+  size_t EnqueuedTUs = 0;
+  size_t IndexedTUs = 0;
----------------
ilya-biryukov wrote:
> Maybe use `std::atomic` instead?
+1 for atomic


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59605