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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 21 07:19:01 PDT 2019


kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:13
+
+#include "/usr/local/google/home/kadircet/repos/llvm/clang-tools-extra/clangd/Context.h"
+#include "/usr/local/google/home/kadircet/repos/llvm/clang-tools-extra/clangd/Logger.h"
----------------
ilya-biryukov wrote:
> Eugene.Zelenko wrote:
> > Please use relative path. Same below.
> Heh, where do these come from?
> Does our include insertion prefer to add global paths in some cases? Dynamic index?
ah interesting, my guess is static index without any "-I" flags to direct path shortening.


================
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);
----------------
jkorous wrote:
> Nit: maybe we should give this constant a name? Or maybe create a command line option for this?
It is the period for building index data structures which helps making queries fast, as I mentioned in the comment we definitely don't need this to happen until we've indexed every file, and BackgroundIndex should in my opinion work in this mode if period was set to zero. So I don't see any point in giving it a name.

Btw, the reason we want to build index data structures in the end is to just see how long it takes.


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:29
 #include "llvm/Support/SHA1.h"
 
 #include <chrono>
----------------
Eugene.Zelenko wrote:
> Unnecessary empty line.
not introduced by this change


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:605
         continue;
+      --UpToDateTUs;
       // FIXME: Currently, we simply schedule indexing on a TU whenever any of
----------------
jkorous wrote:
> 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?
that line is executed at most once per each increment above, if you follow the flow you can see the break below, but good point moving it next to break to better illustrate that.


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:622
+    IndexedTUs += UpToDateTUs;
+    log("[{0}/{1}] Loaded shards from storage", IndexedTUs, EnqueuedTUs);
+  }
----------------
ilya-biryukov wrote:
> Everything else is `vlog` (or even `dlog`), what's the rationale of using `log` here?
It was to print output mentioned in the summary. What would you rather suggest? Other possibilities I had in mind:
- Poll from client for current status
- Pass an option to background index to dump these logs conditionally


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59605





More information about the cfe-commits mailing list