ilya-biryukov added a comment.
Leaving a summary of the offline discussion here.
@sammccall has pointed out maintaining and shipping more tools is work and we
should probably avoid adding them upstream without careful design.
We ended up deciding to **not** land this particular upstream just yet
Replies inline.
On Thu, Apr 11, 2019 at 7:59 PM Sam McCall wrote:
> Thanks - I think there's quite a lot of background missing here:
>
Indeed, here are clarifications.
> what/who is this for,
>
Building the index for clangd without actually running clangd it seems
useful, e.g. for cases when yo
Thanks - I think there's quite a lot of background missing here: what/who
is this for, why a separate binary from clangd-indexer (or and how do they
relate, how much of the BackgroundIndex code structure makes sense with
multiple users.
I assume there's been some offline discussion about this, but
ilya-biryukov added a comment.
To make it clear, I think the question is not just "which part of functionality
is missing in BackgroundIndex", it's rather "which part of BackgroundIndex we
**don't** need".
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.
ilya-biryukov added a subscriber: sammccall.
ilya-biryukov added a comment.
To reiterate the offline discussion: the tool seems useful to me, but it would
be best to keep the client code simpler, it's currently fighting with
`BackgroundIndex` because it's trying to hijack some of its functionali
kadircet marked an inline comment as done.
kadircet added a comment.
Looking at the current state of `BackgroundIndex`, it has the following
implementation details:
- Loading of shards from storage
- Storing of shards to storage
- Collecting symbols from a TU
- Sharding of symbol information col
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
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/c
kadircet updated this revision to Diff 191684.
kadircet marked 11 inline comments as done.
kadircet added a comment.
Herald added a subscriber: jfb.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59605/new/
https://reviews.llvm.or
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 shou
Eugene.Zelenko added inline comments.
Comment at: clang-tools-extra/clangd/index/Background.cpp:29
#include "llvm/Support/SHA1.h"
#include
Unnecessary empty line.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D
ilya-biryukov 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-ex
Eugene.Zelenko 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-e
kadircet created this revision.
kadircet added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jdoerfert, erik.pilkington, arphaman,
jkorous, MaskRay, ioeric, mgorny.
Herald added a project: clang.
Sample output:
I[18:37:50.083] BackgroundIndex: build symbol index periodically e
14 matches
Mail list logo
