[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE344513: [clangd] Minimal implementation of automatic
static index (not enabled). (authored by sammccall, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D53032?vs=169694=169695#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53032
Files:
clangd/CMakeLists.txt
clangd/index/Background.cpp
clangd/index/Background.h
unittests/clangd/BackgroundIndexTests.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -17,7 +17,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H
#include "ClangdServer.h"
-#include
+#include "index/Index.h"
namespace clang {
namespace clangd {
@@ -50,6 +50,9 @@
llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File);
+SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
+SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest );
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
Annotations.cpp
+ BackgroundIndexTests.cpp
CancellationTests.cpp
ClangdTests.cpp
ClangdUnitTests.cpp
Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -0,0 +1,37 @@
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "index/Background.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::UnorderedElementsAre;
+
+namespace clang {
+namespace clangd {
+
+MATCHER_P(Named, N, "") { return arg.Name == N; }
+
+TEST(BackgroundIndexTest, IndexTwoFiles) {
+ MockFSProvider FS;
+ // a.h yields different symbols when included by A.cc vs B.cc.
+ // Currently we store symbols for each TU, so we get both.
+ FS.Files[testPath("root/A.h")] = "void a_h(); void NAME(){}";
+ FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";
+ FS.Files[testPath("root/B.cc")] = "#define NAME bar\n#include \"A.h\"";
+ BackgroundIndex Idx(Context::empty(), "", FS);
+
+ tooling::CompileCommand Cmd;
+ Cmd.Filename = testPath("root/A.cc");
+ Cmd.Directory = testPath("root");
+ Cmd.CommandLine = {"clang++", "-DNAME=foo", testPath("root/A.cc")};
+ Idx.enqueue(testPath("root"), Cmd);
+ Cmd.CommandLine.back() = Cmd.Filename = testPath("root/B.cc");
+ Idx.enqueue(testPath("root"), Cmd);
+
+ Idx.blockUntilIdleForTest();
+ EXPECT_THAT(runFuzzyFind(Idx, ""),
+ UnorderedElementsAre(Named("a_h"), Named("foo"), Named("bar")));
+}
+
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -125,5 +125,17 @@
return std::move(*Result);
}
+SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query) {
+ FuzzyFindRequest Req;
+ Req.Query = Query;
+ return runFuzzyFind(Index, Req);
+}
+
+SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest ) {
+ SymbolSlab::Builder Builder;
+ Index.fuzzyFind(Req, [&](const Symbol ) { Builder.insert(Sym); });
+ return std::move(Builder).build();
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -0,0 +1,79 @@
+//===--- Background.h - Build an index in a background thread *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
+
+#include "Context.h"
+#include "FSProvider.h"
+#include "index/FileIndex.h"
+#include "index/Index.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/Support/SHA1.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+// Builds an in-memory index by by running the static indexer action over
+// all commands in a compilation database. Indexing happens in the background.
+// FIXME: it should also persist its state on disk for fast start.
+// FIXME: it should watch for changes to files on disk.
+class BackgroundIndex : public SwapIndex {
+public:
+ // FIXME: resource-dir injection should be hoisted somewhere common.
+ BackgroundIndex(Context
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
ioeric accepted this revision.
ioeric added a comment.
still lgtm
Comment at: clangd/Compiler.cpp:83
+std::string getStandardResourceDir() {
+ static int Dummy; // Just an address in this process.
Maybe also revert this change?
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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
sammccall updated this revision to Diff 169694.
sammccall marked an inline comment as done.
sammccall added a comment.
Address comments, strip out of clangdlspserver for now.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53032
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/Compiler.cpp
clangd/Compiler.h
clangd/index/Background.cpp
clangd/index/Background.h
unittests/clangd/BackgroundIndexTests.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -17,7 +17,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H
#include "ClangdServer.h"
-#include
+#include "index/Index.h"
namespace clang {
namespace clangd {
@@ -50,6 +50,9 @@
llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File);
+SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
+SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest );
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -125,5 +125,17 @@
return std::move(*Result);
}
+SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query) {
+ FuzzyFindRequest Req;
+ Req.Query = Query;
+ return runFuzzyFind(Index, Req);
+}
+
+SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest ) {
+ SymbolSlab::Builder Builder;
+ Index.fuzzyFind(Req, [&](const Symbol ) { Builder.insert(Sym); });
+ return std::move(Builder).build();
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
Annotations.cpp
+ BackgroundIndexTests.cpp
CancellationTests.cpp
ClangdTests.cpp
ClangdUnitTests.cpp
Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- /dev/null
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -0,0 +1,37 @@
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "index/Background.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::UnorderedElementsAre;
+
+namespace clang {
+namespace clangd {
+
+MATCHER_P(Named, N, "") { return arg.Name == N; }
+
+TEST(BackgroundIndexTest, IndexTwoFiles) {
+ MockFSProvider FS;
+ // a.h yields different symbols when included by A.cc vs B.cc.
+ // Currently we store symbols for each TU, so we get both.
+ FS.Files[testPath("root/A.h")] = "void a_h(); void NAME(){}";
+ FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";
+ FS.Files[testPath("root/B.cc")] = "#define NAME bar\n#include \"A.h\"";
+ BackgroundIndex Idx(Context::empty(), "", FS);
+
+ tooling::CompileCommand Cmd;
+ Cmd.Filename = testPath("root/A.cc");
+ Cmd.Directory = testPath("root");
+ Cmd.CommandLine = {"clang++", "-DNAME=foo", testPath("root/A.cc")};
+ Idx.enqueue(testPath("root"), Cmd);
+ Cmd.CommandLine.back() = Cmd.Filename = testPath("root/B.cc");
+ Idx.enqueue(testPath("root"), Cmd);
+
+ Idx.blockUntilIdleForTest();
+ EXPECT_THAT(runFuzzyFind(Idx, ""),
+ UnorderedElementsAre(Named("a_h"), Named("foo"), Named("bar")));
+}
+
+} // namespace clangd
+} // namespace clang
Index: clangd/index/Background.h
===
--- /dev/null
+++ clangd/index/Background.h
@@ -0,0 +1,79 @@
+//===--- Background.h - Build an index in a background thread *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
+
+#include "Context.h"
+#include "FSProvider.h"
+#include "index/FileIndex.h"
+#include "index/Index.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/Support/SHA1.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+// Builds an in-memory index by by running the static indexer action over
+// all commands in a compilation database. Indexing happens in the background.
+// FIXME: it should also persist its state on disk for fast start.
+// FIXME: it should watch for changes to files on disk.
+class BackgroundIndex : public SwapIndex {
+public:
+ // FIXME: resource-dir injection should be hoisted somewhere common.
+ BackgroundIndex(Context BackgroundContext, StringRef ResourceDir,
+ const FileSystemProvider &);
+
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
sammccall added inline comments.
Comment at: unittests/clangd/BackgroundIndexTests.cpp:14
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+ MockFSProvider FS;
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.
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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
ioeric added inline comments.
Comment at: unittests/clangd/BackgroundIndexTests.cpp:14
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+ MockFSProvider FS;
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.
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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
sammccall added inline comments.
Comment at: unittests/clangd/BackgroundIndexTests.cpp:14
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+ MockFSProvider FS;
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...
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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
ioeric added inline comments.
Comment at: unittests/clangd/BackgroundIndexTests.cpp:14
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+ MockFSProvider FS;
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.
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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
sammccall added inline comments.
Comment at: unittests/clangd/BackgroundIndexTests.cpp:14
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+ MockFSProvider FS;
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...
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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
ioeric accepted this revision.
ioeric added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/index/Background.cpp:51
+ std::unique_lock Lock(QueueMu);
+ assert(!HasActiveTask);
+ QueueCV.wait(Lock, [&] { return ShouldStop || !Queue.empty(); });
Maybe generalize this to `NumActiveTasks`? Currently, this is single-threaded
and safe, but it could be missed when we add more threads.
Comment at: unittests/clangd/BackgroundIndexTests.cpp:14
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+ MockFSProvider FS;
Also add a test for `enqueueAll` with multiple TUs ?
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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
sammccall added inline comments. Comment at: clangd/ClangdLSPServer.h:117 +llvm::Optional CompileCommandsDir, +std::function); ioeric wrote: > please document what the callback is for and how often it's called. Documented at the callsite and in GlobalCompilationDatabase.h. This class here is really just plumbing (it shouldn't be in the header at all) 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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
sammccall updated this revision to Diff 169400.
sammccall added a comment.
Oops, forgot one comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53032
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/Compiler.cpp
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/index/Background.cpp
clangd/index/Background.h
clangd/tool/ClangdMain.cpp
unittests/clangd/BackgroundIndexTests.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -17,7 +17,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H
#include "ClangdServer.h"
-#include
+#include "index/Index.h"
namespace clang {
namespace clangd {
@@ -50,6 +50,9 @@
llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File);
+SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
+SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest );
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -125,5 +125,19 @@
return std::move(*Result);
}
+SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query) {
+ FuzzyFindRequest Req;
+ Req.Query = Query;
+ return runFuzzyFind(Index, Req);
+}
+
+SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest ) {
+ SymbolSlab::Builder Builder;
+ Index.fuzzyFind(Req, [&](const Symbol& Sym) {
+Builder.insert(Sym);
+ });
+ return std::move(Builder).build();
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
Annotations.cpp
+ BackgroundIndexTests.cpp
CancellationTests.cpp
ClangdTests.cpp
ClangdUnitTests.cpp
Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- /dev/null
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -0,0 +1,30 @@
+#include "index/Background.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::ElementsAre;
+
+namespace clang {
+namespace clangd {
+
+MATCHER_P(Named, N, "") { return arg.Name == N; }
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+ MockFSProvider FS;
+ FS.Files[testPath("root/A.h")] = "void NAME(){}";
+ FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";
+ BackgroundIndex Idx(Context::empty(), "", FS);
+ tooling::CompileCommand Cmd;
+ Cmd.Filename = testPath("root/A.cc");
+ Cmd.Directory = testPath("root");
+ Cmd.CommandLine = {"clang++", "-DNAME=foo", testPath("root/A.cc")};
+ Idx.enqueue(testPath("root"), std::move(Cmd));
+
+ Idx.blockUntilIdleForTest();
+ EXPECT_THAT(runFuzzyFind(Idx, "f"), ElementsAre(Named("foo")));
+}
+
+} // namespace clangd
+} // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -167,6 +167,15 @@
"eventually. Don't rely on it."),
llvm::cl::init(""), llvm::cl::Hidden);
+static llvm::cl::opt AutoIndex(
+"auto-index",
+llvm::cl::desc(
+"Build a full index for the codebase containing edited files. "
+"Indexing will occur in the background. "
+"This option is still experimental, as the indexing is inefficient."
+"Takes precedence over -index-file."),
+llvm::cl::init(false), llvm::cl::Hidden);
+
enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };
static llvm::cl::opt CompileArgsFrom(
"compile_args_from", llvm::cl::desc("The source of compile commands"),
@@ -316,9 +325,10 @@
CCOpts.AllScopes = AllScopesCompletion;
// Initialize and run ClangdLSPServer.
- ClangdLSPServer LSPServer(
- Out, CCOpts, CompileCommandsDirPath,
- /*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
+ ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath,
+/*ShouldUseInMemoryCDB=*/CompileArgsFrom ==
+LSPCompileArgs,
+AutoIndex, Opts);
constexpr int NoShutdownRequestErrorCode = 1;
llvm::set_thread_name("clangd.main");
// Change stdin to binary to not lose \r\n on windows.
Index: clangd/index/Background.h
===
--- /dev/null
+++ clangd/index/Background.h
@@ -0,0 +1,79 @@
+//===--- Background.h - Build an index in a background thread
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
sammccall updated this revision to Diff 169399.
sammccall marked 4 inline comments as done.
sammccall added a comment.
Address comments, add tests.
Some changes (enqueue(), blockUntilIdleForTest()) to support testing.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53032
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/Compiler.cpp
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/index/Background.cpp
clangd/index/Background.h
clangd/tool/ClangdMain.cpp
unittests/clangd/BackgroundIndexTests.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -17,7 +17,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H
#include "ClangdServer.h"
-#include
+#include "index/Index.h"
namespace clang {
namespace clangd {
@@ -50,6 +50,9 @@
llvm::Expected>
runDocumentSymbols(ClangdServer , PathRef File);
+SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query);
+SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest );
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -125,5 +125,19 @@
return std::move(*Result);
}
+SymbolSlab runFuzzyFind(const SymbolIndex , StringRef Query) {
+ FuzzyFindRequest Req;
+ Req.Query = Query;
+ return runFuzzyFind(Index, Req);
+}
+
+SymbolSlab runFuzzyFind(const SymbolIndex , const FuzzyFindRequest ) {
+ SymbolSlab::Builder Builder;
+ Index.fuzzyFind(Req, [&](const Symbol& Sym) {
+Builder.insert(Sym);
+ });
+ return std::move(Builder).build();
+}
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -10,6 +10,7 @@
add_extra_unittest(ClangdTests
Annotations.cpp
+ BackgroundIndexTests.cpp
CancellationTests.cpp
ClangdTests.cpp
ClangdUnitTests.cpp
Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- /dev/null
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -0,0 +1,30 @@
+#include "index/Background.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::ElementsAre;
+
+namespace clang {
+namespace clangd {
+
+MATCHER_P(Named, N, "") { return arg.Name == N; }
+
+TEST(BackgroundIndexTest, IndexesOneFile) {
+ MockFSProvider FS;
+ FS.Files[testPath("root/A.h")] = "void NAME(){}";
+ FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";
+ BackgroundIndex Idx(Context::empty(), "", FS);
+ tooling::CompileCommand Cmd;
+ Cmd.Filename = testPath("root/A.cc");
+ Cmd.Directory = testPath("root");
+ Cmd.CommandLine = {"clang++", "-DNAME=foo", testPath("root/A.cc")};
+ Idx.enqueue(testPath("root"), std::move(Cmd));
+
+ Idx.blockUntilIdleForTest();
+ EXPECT_THAT(runFuzzyFind(Idx, "f"), ElementsAre(Named("foo")));
+}
+
+} // namespace clangd
+} // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -167,6 +167,15 @@
"eventually. Don't rely on it."),
llvm::cl::init(""), llvm::cl::Hidden);
+static llvm::cl::opt AutoIndex(
+"auto-index",
+llvm::cl::desc(
+"Build a full index for the codebase containing edited files. "
+"Indexing will occur in the background. "
+"This option is still experimental, as the indexing is inefficient."
+"Takes precedence over -index-file."),
+llvm::cl::init(false), llvm::cl::Hidden);
+
enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };
static llvm::cl::opt CompileArgsFrom(
"compile_args_from", llvm::cl::desc("The source of compile commands"),
@@ -316,9 +325,10 @@
CCOpts.AllScopes = AllScopesCompletion;
// Initialize and run ClangdLSPServer.
- ClangdLSPServer LSPServer(
- Out, CCOpts, CompileCommandsDirPath,
- /*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
+ ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath,
+/*ShouldUseInMemoryCDB=*/CompileArgsFrom ==
+LSPCompileArgs,
+AutoIndex, Opts);
constexpr int NoShutdownRequestErrorCode = 1;
llvm::set_thread_name("clangd.main");
// Change stdin to binary to not lose \r\n on windows.
Index: clangd/index/Background.h
===
---
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
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 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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
ioeric added a comment.
Looks good in general.
Comment at: clangd/ClangdLSPServer.h:117
+llvm::Optional CompileCommandsDir,
+std::function);
please document what the callback is for and how often it's called.
Comment at: clangd/index/Background.cpp:63
+ const tooling::CompilationDatabase ) {
+ // TODO: this function may be slow. Perhaps enqueue a task to re-read the CDB
+ // from disk and enqueue the commands asynchronously?
Maybe add a trace?
Comment at: clangd/index/Background.cpp:98
+ }
+ llvm::sys::path::native(AbsolutePath);
+
Why is this needed?
Comment at: clangd/index/Background.cpp:163
+ vlog("Rebuilding automatic index");
+ reset(IndexedSymbols.buildMemIndex());
+ return Error::success();
Add a FIXME to use Dex?
Comment at: clangd/index/Background.h:1
+//===--- Background.h - Build an index in a background thread *-
C++-*-===//
+//
Is it possible to add test for this?
Comment at: clangd/index/Background.h:28
+// all commands in a compilation database. Indexing happens in the background.
+// TODO: it should also persist its state on disk for fast start.
+class BackgroundIndex : public SwapIndex {
s/TODO/FIXME/?
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,
Maybe add a FIXME for files not in CDB e.g. headers?
Comment at: clangd/tool/ClangdMain.cpp:170
+static llvm::cl::opt AutoIndex(
+"auto-index",
Should we ignore index-file/auto-index if one is set?
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
[PATCH] D53032: [clangd] Minimal implementation of automatic static index, behind a flag.
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, jfb, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov, mgorny.
See tinyurl.com/clangd-automatic-index for design and goals.
Lots of limitations to keep this patch smallish, TODOs everywhere:
- no serialization to disk
- no changes to dynamic index, which now has a much simpler job
- no partitioning of symbols by file to avoid duplication of header symbols
- no reindexing of edited files
- only a single worker thread
- compilation database is slurped synchronously (doesn't scale)
- uses memindex, rebuilds after every file (should be dex, periodically)
Still needs tests, but should be ready for review of the basic shape.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53032
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/Compiler.cpp
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/index/Background.cpp
clangd/index/Background.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -167,6 +167,14 @@
"eventually. Don't rely on it."),
llvm::cl::init(""), llvm::cl::Hidden);
+static llvm::cl::opt AutoIndex(
+"auto-index",
+llvm::cl::desc(
+"Build a full index for the codebase containing edited files. "
+"Indexing will occur in the background. "
+"This option is still experimental, as the indexing is inefficient."),
+llvm::cl::init(false), llvm::cl::Hidden);
+
enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };
static llvm::cl::opt CompileArgsFrom(
"compile_args_from", llvm::cl::desc("The source of compile commands"),
@@ -316,9 +324,10 @@
CCOpts.AllScopes = AllScopesCompletion;
// Initialize and run ClangdLSPServer.
- ClangdLSPServer LSPServer(
- Out, CCOpts, CompileCommandsDirPath,
- /*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
+ ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath,
+/*ShouldUseInMemoryCDB=*/CompileArgsFrom ==
+LSPCompileArgs,
+AutoIndex, Opts);
constexpr int NoShutdownRequestErrorCode = 1;
llvm::set_thread_name("clangd.main");
// Change stdin to binary to not lose \r\n on windows.
Index: clangd/index/Background.h
===
--- /dev/null
+++ clangd/index/Background.h
@@ -0,0 +1,73 @@
+//===--- Background.h - Build an index in a background thread *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_H
+
+#include "Context.h"
+#include "FSProvider.h"
+#include "index/Index.h"
+#include "index/FileIndex.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/Support/SHA1.h"
+#include
+#include
+#include
+
+namespace clang {
+namespace clangd {
+
+// Builds an in-memory index by by running the static indexer action over
+// all commands in a compilation database. Indexing happens in the background.
+// TODO: it should also persist its state on disk for fast start.
+class BackgroundIndex : public SwapIndex {
+public:
+ // TODO: FileSystemProvider is not const-correct.
+ // TODO: resource-dir injection should be hoisted somewhere common.
+ BackgroundIndex(Context BackgroundContext,
+ StringRef ResourceDir, FileSystemProvider *);
+ ~BackgroundIndex(); // Blocks while the current task finishes.
+
+ // Index all TUs described in the compilation database.
+ // 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,
+ const tooling::CompilationDatabase &);
+
+ // Cause background threads to stop after ther current task, any remaining
+ // tasks will be discarded.
+ void stop();
+
+private:
+ // configuration
+ std::string ResourceDir;
+ FileSystemProvider *FSProvider;
+ Context BackgroundContext;
+
+ // index state
+ llvm::Error index(tooling::CompileCommand);
+ FileSymbols IndexedSymbols; // Index contents.
+ using Hash = decltype(llvm::SHA1::hash({}));
+ llvm::StringMap FileHash; // Digest of indexed file.
+
+ // queue management
+ using Task = std::function; // TODO: use multiple worker threads.
+ void run(); // Main loop executed by Thread. Runs tasks from Queue.
+ void
