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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
