sammccall marked 6 inline comments as done.
sammccall added inline comments.
================
Comment at: clangd/index/Background.cpp:98
+ }
+ llvm::sys::path::native(AbsolutePath);
+
----------------
ioeric wrote:
> Why is this needed?
Removed. I copied it from the related code in JSONCompilationDatabase, but I'm
not sure what the right behavior is, as JSONCompilationDatabase doesn't specify
the format of paths.
================
Comment at: clangd/index/Background.cpp:163
+ vlog("Rebuilding automatic index");
+ reset(IndexedSymbols.buildMemIndex());
+ return Error::success();
----------------
ioeric wrote:
> Add a FIXME to use Dex?
Added it to the existing TODO. They're coupled: dex is expensive to build, so
rebuilding it after every file wouldnt' make sense.
================
Comment at: clangd/index/Background.h:1
+//===--- Background.h - Build an index in a background thread ----*-
C++-*-===//
+//
----------------
ioeric wrote:
> Is it possible to add test for this?
Added a test that indexes a file, waits for the queue to be empty, and queries.
That's pretty basic, do you have suggestions for things you'd like to see
tested?
================
Comment at: clangd/index/Background.h:39
+ // The indexing happens in a background thread, so after enqueueing files
+ // for indexing their symbols will be available sometime later.
+ void enqueueAll(llvm::StringRef Directory,
----------------
ioeric wrote:
> Maybe add a FIXME for files not in CDB e.g. headers?
There's nothing missing for headers at this point.
Headers will already be indexed (as part of any TU that includes them).
Later, we'll partition the outputs by file, but these headers will still be
indexed.
When it comes time to watch files on disk for changes, then we'll need to
consider headers specially.
Indexing headers that are not included by any file is out of scope (it's not
clear this is a good idea, e.g. consider files under test/ in llvm).
================
Comment at: clangd/tool/ClangdMain.cpp:170
+static llvm::cl::opt<bool> AutoIndex(
+ "auto-index",
----------------
ioeric wrote:
> Should we ignore index-file/auto-index if one is set?
Since this is hidden/experimental, I think it's not worth adding explicit code
so they interact nicely, I just documented the behavior instead instead.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53032
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
