[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: unittests/clangd/BackgroundIndexTests.cpp:14
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+  MockFSProvider FS;
----------------
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > ioeric wrote:
> > > > sammccall wrote:
> > > > > ioeric wrote:
> > > > > > Also add a test for `enqueueAll` with multiple TUs ?
> > > > > Is it important to call `enqueueAll` specifically vs `enqueue` multiple times?
> > > > > 
> > > > > We don't have a good test fixture for a compilation database, and `enqueueAll` is trivial...
> > > > I think the randomization code worths a test. 
> > > > 
> > > > How about adding a test in ClangdServer with the auto index enabled? I think we'd also want coverage in ClangdServer anyway.
> > > How would you suggest testing the randomization :-)
> > > 
> > > The problem with a ClangdServer test is that it doesn't know anything about autoindex.
> > > AutoIndex lives in ClangdLSPServer, because that's where the compilation database lives (because people keep cramming LSP extensions in to manipulate it, and I haven't been able to remove them).
> > > Nobody has worked out how to test ClangdLSPServer yet. It's a worthy project, but...
> > > How would you suggest testing the randomization :-)
> > Not suggesting testing the behavior; just test that it works really. I guess a single file would also do it.
> > 
> > > The problem with a ClangdServer test is that it doesn't know anything about autoindex.
> > Ah, I see. That's a bit unfortunate. ClangdServer seems to be a better place for the auto index. I wonder if we could move AutoIndex into ClangdServer? I'm not very familiar with CDB APIs, but intuitively this seems doable if we tweak the interface of CDB a bit e.g. inject the callback into `GlobalCompilationDatabase` without going through `CompilationDB`? Not sure if how well this would play with the CDB design though.
> > ClangdServer seems to be a better place for the auto index
> I fully agree.
> 
> >  I wonder if we could move AutoIndex into ClangdServer? I'm not very familiar with CDB APIs...
> Currently the global CDB is trying to be all things to all people:
>  - by default, it follows Tooling conventions and looks for compile_commands above the source files
>  - google's hosted option wants it to be completely opaque, no indexing
>  - apple wants it to be an in-memory store mutated over LSP
>  - ericsson wants it to be a fixed directory set by the user
> 
> And we've implemented these in ad-hoc ways, not behind any real abstraction. I don't think adding more functionality to the interface without addressing this is a great idea, and I don't really know how to untangle all this without asking people to rethink their use cases. But I'll think about this some more.
After thinking about this, decided to just check in the library with unit tests for now while we sort out the layering.
I've ripped out all the ClangdMain and ClangdLSPServer changes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53032