[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE358400: [clangd] Wait for compile command in ASTWorker
instead of ClangdServer (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D60607?vs=195139&id=195141#toc
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -1099,6 +1099,64 @@
Field(&CodeCompletion::Scope, "ns::";
}
+TEST_F(ClangdVFSTest, FallbackWhenWaitingForCompileCommand) {
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ // Returns compile command only when notified.
+ class DelayedCompilationDatabase : public GlobalCompilationDatabase {
+ public:
+DelayedCompilationDatabase(Notification &CanReturnCommand)
+: CanReturnCommand(CanReturnCommand) {}
+
+llvm::Optional
+getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override {
+ // FIXME: make this timeout and fail instead of waiting forever in case
+ // something goes wrong.
+ CanReturnCommand.wait();
+ auto FileName = llvm::sys::path::filename(File);
+ std::vector CommandLine = {"clangd", "-ffreestanding", File};
+ return {tooling::CompileCommand(llvm::sys::path::parent_path(File),
+ FileName, std::move(CommandLine), "")};
+}
+
+std::vector ExtraClangFlags;
+
+ private:
+Notification &CanReturnCommand;
+ };
+
+ Notification CanReturnCommand;
+ DelayedCompilationDatabase CDB(CanReturnCommand);
+ ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+ auto FooCpp = testPath("foo.cpp");
+ Annotations Code(R"cpp(
+namespace ns { int xyz; }
+using namespace ns;
+int main() {
+ xy^
+})cpp");
+ FS.Files[FooCpp] = FooCpp;
+ Server.addDocument(FooCpp, Code.code());
+
+ // Sleep for some time to make sure code completion is not run because update
+ // hasn't been scheduled.
+ std::this_thread::sleep_for(std::chrono::milliseconds(10));
+ auto Opts = clangd::CodeCompleteOptions();
+ Opts.AllowFallback = true;
+
+ auto Res = cantFail(runCodeComplete(Server, FooCpp, Code.point(), Opts));
+ EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+
+ CanReturnCommand.notify();
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
+ EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
+ clangd::CodeCompleteOptions()))
+ .Completions,
+ ElementsAre(AllOf(Field(&CodeCompletion::Name, "xyz"),
+Field(&CodeCompletion::Scope, "ns::";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
namespace clangd {
namespace {
-using ::testing::_;
-using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(getDefaultAsyncThreadsCount(),
+ TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -154,7 +152,7 @@
// thread until we've scheduled them all.
Notification Ready;
TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -184,7 +182,7 @@
TEST_F(TUSchedulerTests, Debounce) {
std::atomic CallbackCount(0);
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
@@ -220,7 +218,7 @@
{
Notification InconsistentReadDone; // Must live longest.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric updated this revision to Diff 195139.
ioeric marked 5 inline comments as done.
ioeric added a comment.
- use sync runAddDocument in test
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
namespace clangd {
namespace {
-using ::testing::_;
-using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(getDefaultAsyncThreadsCount(),
+ TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -154,7 +152,7 @@
// thread until we've scheduled them all.
Notification Ready;
TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -184,7 +182,7 @@
TEST_F(TUSchedulerTests, Debounce) {
std::atomic CallbackCount(0);
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
@@ -220,7 +218,7 @@
{
Notification InconsistentReadDone; // Must live longest.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -277,7 +275,7 @@
{
Notification Proceed; // Ensure we schedule everything.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -346,7 +344,7 @@
// Run TUScheduler and collect some stats.
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -437,10 +435,11 @@
std::atomic BuiltASTCounter(0);
ASTRetentionPolicy Policy;
Policy.MaxRetainedASTs = 2;
- TUScheduler S(
- /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
llvm::StringLiteral SourceContents = R"cpp(
int* a;
@@ -487,11 +486,11 @@
}
TEST_F(TUSchedulerTests, EmptyPreamble) {
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
// Testing strategy: we update the file and schedule a few preamble reads at
// the same time. All reads should get the same non-null preamble.
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemo
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM! See the NITs, specifically about `runAddDocument` - those definitely look
worth fixing.
Comment at: unittests/clangd/ClangdTests.cpp:1148
+ EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+ // Identifier-based fallback completion doesn't know about "symbol" scope.
+ EXPECT_THAT(Res.Completions,
ioeric wrote:
> ilya-biryukov wrote:
> > Not strictly related to this patch, but maybe we could add a flag to
> > completion results to indicate if the completion happened via a fallback
> > mode or not?
> >
> > Would make the test code more straightforward and the tests like these
> > would not rely on a particular implementation of the fallback mode (e.g. I
> > can imagine the fallback mode learning about the scopes later on)
> We are setting the context to `Recovery` and make fallback as part of
> `Recovery`. Do you we should distinguish fallback mode from `Recovery`?
Ah, `Recovery` looks good enough if we check the same location twice and second
should be non-recovery.
Maybe keep **only** the `Context == Recovery` check? Checking for particular
results only seems to make test less focused.
Comment at: unittests/clangd/ClangdTests.cpp:1113
+getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override {
+ CanReturnCommand.wait();
+ auto FileName = llvm::sys::path::filename(File);
Ah, we should really have a wait-with-timeout for these use-cases.
It's sad that we'll block indefinitely in the old implementation at this point.
Having a failing test is much better than the one that never finishes.
No need to do anything in this patch, though, that requires a change to
`Threading.h` that is best done separately anyway.
Comment at: unittests/clangd/CodeCompleteTests.cpp:1392
+ // code completion.
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
CodeCompleteResult Completions = cantFail(runCodeComplete(
Same here: could you use `runAddDocument`?
Comment at: unittests/clangd/CodeCompleteTests.cpp:1390
Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
CodeCompleteResult Completions = cantFail(runCodeComplete(
ioeric wrote:
> ilya-biryukov wrote:
> > Could you expand why we need this?
> Added a comment.
Ah, thanks! Could we instead use a sync helper here to keep the code a bit
simpler (won't need a comment too)?
```
runAddDocument(Server, ...);
```
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric updated this revision to Diff 195137.
ioeric marked 9 inline comments as done.
ioeric added a comment.
- address comments
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
namespace clangd {
namespace {
-using ::testing::_;
-using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(getDefaultAsyncThreadsCount(),
+ TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -154,7 +152,7 @@
// thread until we've scheduled them all.
Notification Ready;
TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -184,7 +182,7 @@
TEST_F(TUSchedulerTests, Debounce) {
std::atomic CallbackCount(0);
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
@@ -220,7 +218,7 @@
{
Notification InconsistentReadDone; // Must live longest.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -277,7 +275,7 @@
{
Notification Proceed; // Ensure we schedule everything.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -346,7 +344,7 @@
// Run TUScheduler and collect some stats.
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -437,10 +435,11 @@
std::atomic BuiltASTCounter(0);
ASTRetentionPolicy Policy;
Policy.MaxRetainedASTs = 2;
- TUScheduler S(
- /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
llvm::StringLiteral SourceContents = R"cpp(
int* a;
@@ -487,11 +486,11 @@
}
TEST_F(TUSchedulerTests, EmptyPreamble) {
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
// Testing strategy: we update the file and schedule a few preamble reads at
// the same time. All reads should get the same non-null preamble.
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric added inline comments. Comment at: unittests/clangd/ClangdTests.cpp:1148 + EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery); + // Identifier-based fallback completion doesn't know about "symbol" scope. + EXPECT_THAT(Res.Completions, ilya-biryukov wrote: > Not strictly related to this patch, but maybe we could add a flag to > completion results to indicate if the completion happened via a fallback mode > or not? > > Would make the test code more straightforward and the tests like these would > not rely on a particular implementation of the fallback mode (e.g. I can > imagine the fallback mode learning about the scopes later on) We are setting the context to `Recovery` and make fallback as part of `Recovery`. Do you we should distinguish fallback mode from `Recovery`? Comment at: unittests/clangd/CodeCompleteTests.cpp:1390 Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes); + ASSERT_TRUE(Server.blockUntilIdleForTest()); CodeCompleteResult Completions = cantFail(runCodeComplete( ilya-biryukov wrote: > Could you expand why we need this? Added a comment. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60607/new/ https://reviews.llvm.org/D60607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ilya-biryukov added a comment.
Thanks, the change LG now. Only nits from my side!
Comment at: clangd/Compiler.h:19
#include "../clang-tidy/ClangTidyOptions.h"
+#include "GlobalCompilationDatabase.h"
#include "index/Index.h"
NIT: this looks unrelated to the actual change. Maybe revert?
Comment at: clangd/TUScheduler.cpp:224
+ std::shared_ptr getCurrentFileInputs() const;
+
Could you add a comment that this is private because `Inputs.FS` is not
thread-safe and the client code should take care to not expose it via a public
interface.
Comment at: clangd/TUScheduler.cpp:255
mutable std::mutex Mutex;
+ /// Inputs, corresponding to the current state of AST.
+ std::shared_ptr FileInputs; /* GUARDED_BY(Mutex)
*/
This comment is not true anymore, the `FileInputs` might be out-of-sync with
the AST for short spans of time.
Maybe something like:
```
/// File inputs, currently being used by the worker.
/// Inputs are written and read by the worker thread, compile command can also
be consumed by clients of ASTWorker.
```
Comment at: clangd/TUScheduler.cpp:343
+ auto Inputs = std::make_shared();
+ Inputs->CompileCommand = CDB.getFallbackCommand(FileName);
+ FileInputs = std::move(Inputs);
NIT: maybe add a comment explaining why only `CompileCommand` is set and not
the other fields?
Thinking of something like:
```
// Other fields are never read outside worker thread and the worker thread will
initialize them before first use.
```
Comment at: clangd/TUScheduler.cpp:360
auto Task = [=]() mutable {
+// Get the actual command as `Inputs` contains fallback command.
+// FIXME: some build systems like Bazel will take time to preparing
IIUC, The comment does not correspond to the latest version.
s/contains fallback command/does not have a command.
Comment at: clangd/TUScheduler.cpp:380
+ std::lock_guard Lock(Mutex);
+ FileInputs.reset(new ParseInputs(Inputs));
+}
NIT: maybe avoid using new?
```FileInputs = std::make_shared(Inputs)```
Feel free to keep as is too, I find the proposed style simpler to follow, but
you could reasonably disagree.
Comment at: unittests/clangd/ClangdTests.cpp:1148
+ EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+ // Identifier-based fallback completion doesn't know about "symbol" scope.
+ EXPECT_THAT(Res.Completions,
Not strictly related to this patch, but maybe we could add a flag to completion
results to indicate if the completion happened via a fallback mode or not?
Would make the test code more straightforward and the tests like these would
not rely on a particular implementation of the fallback mode (e.g. I can
imagine the fallback mode learning about the scopes later on)
Comment at: unittests/clangd/CodeCompleteTests.cpp:1390
Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
CodeCompleteResult Completions = cantFail(runCodeComplete(
Could you expand why we need this?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric updated this revision to Diff 195111.
ioeric added a comment.
- Only return compile command (instead of FileInputs) from AST worker.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
namespace clangd {
namespace {
-using ::testing::_;
-using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(getDefaultAsyncThreadsCount(),
+ TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -154,7 +152,7 @@
// thread until we've scheduled them all.
Notification Ready;
TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -184,7 +182,7 @@
TEST_F(TUSchedulerTests, Debounce) {
std::atomic CallbackCount(0);
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
@@ -220,7 +218,7 @@
{
Notification InconsistentReadDone; // Must live longest.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -277,7 +275,7 @@
{
Notification Proceed; // Ensure we schedule everything.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -346,7 +344,7 @@
// Run TUScheduler and collect some stats.
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -437,10 +435,11 @@
std::atomic BuiltASTCounter(0);
ASTRetentionPolicy Policy;
Policy.MaxRetainedASTs = 2;
- TUScheduler S(
- /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
llvm::StringLiteral SourceContents = R"cpp(
int* a;
@@ -487,11 +486,11 @@
}
TEST_F(TUSchedulerTests, EmptyPreamble) {
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
// Testing strategy: we update the file and schedule a few preamble reads at
// the same time. All reads should get the same non-null preamble.
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric marked an inline comment as done.
ioeric added inline comments.
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > ilya-biryukov wrote:
> > > > > > ioeric wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > This is racy, `FileInputs` is only accessed from a worker
> > > > > > > > thread now.
> > > > > > > > I'm afraid we'll need a separate variable with a lock around it
> > > > > > > > (could reuse one lock for preamble and compile command,
> > > > > > > > probably)
> > > > > > > It looks like we could also guard `FileInputs`? Adding another
> > > > > > > variable seems to make the state more complicated.
> > > > > > `FileInputs` are currently accessed without locks in a bunch of
> > > > > > places and it'd be nice to keep it that way (the alternative is
> > > > > > more complicated code).
> > > > > > Doing the same thing we do for preamble looks like the best
> > > > > > alternative here.
> > > > > The reason I think it would be easier to guard `FileInputs` with
> > > > > mutex instead of pulling a new variable is that CompileCommand is
> > > > > usually used along with other fields in `FileInputs`. I think we
> > > > > could manage this with relatively few changes. Please take a look at
> > > > > the new changes.
> > > > Unfortunately we can't share `Inputs.FS` safely as the vfs
> > > > implementations are not thread-safe.
> > > It seems to me that the behavior for `FS` hasn't change in this patch.
> > > And `FileInputs` (incl. `Inputs.FS`) has always been available/shared
> > > across worker threads. We don't seem to have systematic way to prevent
> > > raciness before this patch, and it would be good to have it somehow, but
> > > it's probably out of the scope.
> > >
> > > Maybe I'm missing something or misinterpreting. Could you elaborate?
> > > And FileInputs (incl. Inputs.FS) has always been available/shared across
> > > worker threads
> > I don't think that's the case, we did not a public API to get the hands on
> > `ASTWorker::FileInputs.FS` and we're getting one now.
> > Even though the current patch does not add such racy accesses, it certainly
> > makes it much easier to do it accidentally from the clients of `ASTWorker`
> > in the future (one just needs to access `getCurrentInputs().FS`).
> >
> > The (not very) systematic way to prevent raciness that we use now is to
> > **not** protect the members which are potentially racy with locks and
> > document they are accessed only from a worker thread.
> > Having a separate copy of the compile command is a small price to pay (both
> > in terms of performance and complexity) to avoid exposing non-thread-safe
> > members of `ASTWorker` in its public interface.
> I don't think that makes a fundamental difference as `FileInputs.FS` is
> already racy. But if "public" API is the concern, we could simply restrict
> the scope of the API and make return `CompileCommand` only.
Inside `ASTWorker`, `FileInputs` can still be guarded with `Mutex` as a whole.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric marked an inline comment as done.
ioeric added inline comments.
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > ilya-biryukov wrote:
> > > > > ioeric wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > This is racy, `FileInputs` is only accessed from a worker thread
> > > > > > > now.
> > > > > > > I'm afraid we'll need a separate variable with a lock around it
> > > > > > > (could reuse one lock for preamble and compile command, probably)
> > > > > > It looks like we could also guard `FileInputs`? Adding another
> > > > > > variable seems to make the state more complicated.
> > > > > `FileInputs` are currently accessed without locks in a bunch of
> > > > > places and it'd be nice to keep it that way (the alternative is more
> > > > > complicated code).
> > > > > Doing the same thing we do for preamble looks like the best
> > > > > alternative here.
> > > > The reason I think it would be easier to guard `FileInputs` with mutex
> > > > instead of pulling a new variable is that CompileCommand is usually
> > > > used along with other fields in `FileInputs`. I think we could manage
> > > > this with relatively few changes. Please take a look at the new changes.
> > > Unfortunately we can't share `Inputs.FS` safely as the vfs
> > > implementations are not thread-safe.
> > It seems to me that the behavior for `FS` hasn't change in this patch. And
> > `FileInputs` (incl. `Inputs.FS`) has always been available/shared across
> > worker threads. We don't seem to have systematic way to prevent raciness
> > before this patch, and it would be good to have it somehow, but it's
> > probably out of the scope.
> >
> > Maybe I'm missing something or misinterpreting. Could you elaborate?
> > And FileInputs (incl. Inputs.FS) has always been available/shared across
> > worker threads
> I don't think that's the case, we did not a public API to get the hands on
> `ASTWorker::FileInputs.FS` and we're getting one now.
> Even though the current patch does not add such racy accesses, it certainly
> makes it much easier to do it accidentally from the clients of `ASTWorker` in
> the future (one just needs to access `getCurrentInputs().FS`).
>
> The (not very) systematic way to prevent raciness that we use now is to
> **not** protect the members which are potentially racy with locks and
> document they are accessed only from a worker thread.
> Having a separate copy of the compile command is a small price to pay (both
> in terms of performance and complexity) to avoid exposing non-thread-safe
> members of `ASTWorker` in its public interface.
I don't think that makes a fundamental difference as `FileInputs.FS` is already
racy. But if "public" API is the concern, we could simply restrict the scope of
the API and make return `CompileCommand` only.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ilya-biryukov added inline comments.
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > ilya-biryukov wrote:
> > > > > > This is racy, `FileInputs` is only accessed from a worker thread
> > > > > > now.
> > > > > > I'm afraid we'll need a separate variable with a lock around it
> > > > > > (could reuse one lock for preamble and compile command, probably)
> > > > > It looks like we could also guard `FileInputs`? Adding another
> > > > > variable seems to make the state more complicated.
> > > > `FileInputs` are currently accessed without locks in a bunch of places
> > > > and it'd be nice to keep it that way (the alternative is more
> > > > complicated code).
> > > > Doing the same thing we do for preamble looks like the best alternative
> > > > here.
> > > The reason I think it would be easier to guard `FileInputs` with mutex
> > > instead of pulling a new variable is that CompileCommand is usually used
> > > along with other fields in `FileInputs`. I think we could manage this
> > > with relatively few changes. Please take a look at the new changes.
> > Unfortunately we can't share `Inputs.FS` safely as the vfs implementations
> > are not thread-safe.
> It seems to me that the behavior for `FS` hasn't change in this patch. And
> `FileInputs` (incl. `Inputs.FS`) has always been available/shared across
> worker threads. We don't seem to have systematic way to prevent raciness
> before this patch, and it would be good to have it somehow, but it's probably
> out of the scope.
>
> Maybe I'm missing something or misinterpreting. Could you elaborate?
> And FileInputs (incl. Inputs.FS) has always been available/shared across
> worker threads
I don't think that's the case, we did not a public API to get the hands on
`ASTWorker::FileInputs.FS` and we're getting one now.
Even though the current patch does not add such racy accesses, it certainly
makes it much easier to do it accidentally from the clients of `ASTWorker` in
the future (one just needs to access `getCurrentInputs().FS`).
The (not very) systematic way to prevent raciness that we use now is to **not**
protect the members which are potentially racy with locks and document they are
accessed only from a worker thread.
Having a separate copy of the compile command is a small price to pay (both in
terms of performance and complexity) to avoid exposing non-thread-safe members
of `ASTWorker` in its public interface.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric marked an inline comment as done.
ioeric added inline comments.
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > ilya-biryukov wrote:
> > > > > This is racy, `FileInputs` is only accessed from a worker thread now.
> > > > > I'm afraid we'll need a separate variable with a lock around it
> > > > > (could reuse one lock for preamble and compile command, probably)
> > > > It looks like we could also guard `FileInputs`? Adding another variable
> > > > seems to make the state more complicated.
> > > `FileInputs` are currently accessed without locks in a bunch of places
> > > and it'd be nice to keep it that way (the alternative is more complicated
> > > code).
> > > Doing the same thing we do for preamble looks like the best alternative
> > > here.
> > The reason I think it would be easier to guard `FileInputs` with mutex
> > instead of pulling a new variable is that CompileCommand is usually used
> > along with other fields in `FileInputs`. I think we could manage this with
> > relatively few changes. Please take a look at the new changes.
> Unfortunately we can't share `Inputs.FS` safely as the vfs implementations
> are not thread-safe.
It seems to me that the behavior for `FS` hasn't change in this patch. And
`FileInputs` (incl. `Inputs.FS`) has always been available/shared across worker
threads. We don't seem to have systematic way to prevent raciness before this
patch, and it would be good to have it somehow, but it's probably out of the
scope.
Maybe I'm missing something or misinterpreting. Could you elaborate?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ilya-biryukov added inline comments.
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > This is racy, `FileInputs` is only accessed from a worker thread now.
> > > > I'm afraid we'll need a separate variable with a lock around it (could
> > > > reuse one lock for preamble and compile command, probably)
> > > It looks like we could also guard `FileInputs`? Adding another variable
> > > seems to make the state more complicated.
> > `FileInputs` are currently accessed without locks in a bunch of places and
> > it'd be nice to keep it that way (the alternative is more complicated code).
> > Doing the same thing we do for preamble looks like the best alternative
> > here.
> The reason I think it would be easier to guard `FileInputs` with mutex
> instead of pulling a new variable is that CompileCommand is usually used
> along with other fields in `FileInputs`. I think we could manage this with
> relatively few changes. Please take a look at the new changes.
Unfortunately we can't share `Inputs.FS` safely as the vfs implementations are
not thread-safe.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric added inline comments.
Comment at: clangd/TUScheduler.cpp:338
+ Barrier(Barrier), Done(false) {
+ FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > The command is filled with a fallback by `ClangdServer`, right? Why do we
> > > need to repeat this here?
> > `ASTWorker::FileInputs` is not set until ASTWorker runs update and gets
> > compile command. Before that, `FileInputs` contains empty compile command
> > that can be used by `runWithPreamble`/`runWithAST` (afaict). Initializing
> > it to fallback command seems to be a better alternative.
> That makes sense. Could we remove the `getFallbackCommand` call from the
> `ClangdServer::addDocument`?
> Let's make the `TUScheduler` fully responsible for the compile command.
Sounds good.
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > This is racy, `FileInputs` is only accessed from a worker thread now.
> > > I'm afraid we'll need a separate variable with a lock around it (could
> > > reuse one lock for preamble and compile command, probably)
> > It looks like we could also guard `FileInputs`? Adding another variable
> > seems to make the state more complicated.
> `FileInputs` are currently accessed without locks in a bunch of places and
> it'd be nice to keep it that way (the alternative is more complicated code).
> Doing the same thing we do for preamble looks like the best alternative here.
The reason I think it would be easier to guard `FileInputs` with mutex instead
of pulling a new variable is that CompileCommand is usually used along with
other fields in `FileInputs`. I think we could manage this with relatively few
changes. Please take a look at the new changes.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric updated this revision to Diff 194891.
ioeric marked 5 inline comments as done.
ioeric added a comment.
- address comments
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
namespace clangd {
namespace {
-using ::testing::_;
-using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(getDefaultAsyncThreadsCount(),
+ TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -154,7 +152,7 @@
// thread until we've scheduled them all.
Notification Ready;
TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -184,7 +182,7 @@
TEST_F(TUSchedulerTests, Debounce) {
std::atomic CallbackCount(0);
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
@@ -220,7 +218,7 @@
{
Notification InconsistentReadDone; // Must live longest.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -277,7 +275,7 @@
{
Notification Proceed; // Ensure we schedule everything.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -346,7 +344,7 @@
// Run TUScheduler and collect some stats.
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -437,10 +435,11 @@
std::atomic BuiltASTCounter(0);
ASTRetentionPolicy Policy;
Policy.MaxRetainedASTs = 2;
- TUScheduler S(
- /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
llvm::StringLiteral SourceContents = R"cpp(
int* a;
@@ -487,11 +486,11 @@
}
TEST_F(TUSchedulerTests, EmptyPreamble) {
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
// Testing strategy: we update the file and schedule a few preamble reads at
// the same time. All reads should get the same non-null preamble.
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ilya-biryukov added inline comments.
Comment at: clangd/TUScheduler.cpp:338
+ Barrier(Barrier), Done(false) {
+ FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}
ioeric wrote:
> ilya-biryukov wrote:
> > The command is filled with a fallback by `ClangdServer`, right? Why do we
> > need to repeat this here?
> `ASTWorker::FileInputs` is not set until ASTWorker runs update and gets
> compile command. Before that, `FileInputs` contains empty compile command
> that can be used by `runWithPreamble`/`runWithAST` (afaict). Initializing it
> to fallback command seems to be a better alternative.
That makes sense. Could we remove the `getFallbackCommand` call from the
`ClangdServer::addDocument`?
Let's make the `TUScheduler` fully responsible for the compile command.
Comment at: clangd/TUScheduler.cpp:359
+if (auto Cmd = CDB.getCompileCommand(FileName))
+ Inputs.CompileCommand = *Cmd;
// Will be used to check if we can avoid rebuilding the AST.
ioeric wrote:
> ilya-biryukov wrote:
> > Maybe update the heuristic field in the current compile command if its
> > empty to indicate we're using an older command?
> > That might happen if a previous call to `getCompileCommand` succeeded (so
> > the stored command does not have a heuristic set).
> If `CDB.getCompileCommand` failed, we would use `Inputs.CompileCommand` which
> should be the fallback command set in ClangdServer. It might be a good idea
> to use the old command, but it doesn't seem to be in the scope of this patch.
> Added a `FIXME`.
Oh, I mixed `Inputs` and `FileInputs` in my head.
It feels reasonable to completely move the handling of compile command to
`TUScheduler`, see the other comment at the constructor of `ASTWorker`.
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
ioeric wrote:
> ilya-biryukov wrote:
> > This is racy, `FileInputs` is only accessed from a worker thread now.
> > I'm afraid we'll need a separate variable with a lock around it (could
> > reuse one lock for preamble and compile command, probably)
> It looks like we could also guard `FileInputs`? Adding another variable seems
> to make the state more complicated.
`FileInputs` are currently accessed without locks in a bunch of places and it'd
be nice to keep it that way (the alternative is more complicated code).
Doing the same thing we do for preamble looks like the best alternative here.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric added inline comments.
Comment at: clangd/TUScheduler.cpp:338
+ Barrier(Barrier), Done(false) {
+ FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}
ilya-biryukov wrote:
> The command is filled with a fallback by `ClangdServer`, right? Why do we
> need to repeat this here?
`ASTWorker::FileInputs` is not set until ASTWorker runs update and gets compile
command. Before that, `FileInputs` contains empty compile command that can be
used by `runWithPreamble`/`runWithAST` (afaict). Initializing it to fallback
command seems to be a better alternative.
Comment at: clangd/TUScheduler.cpp:359
+if (auto Cmd = CDB.getCompileCommand(FileName))
+ Inputs.CompileCommand = *Cmd;
// Will be used to check if we can avoid rebuilding the AST.
ilya-biryukov wrote:
> Maybe update the heuristic field in the current compile command if its empty
> to indicate we're using an older command?
> That might happen if a previous call to `getCompileCommand` succeeded (so the
> stored command does not have a heuristic set).
If `CDB.getCompileCommand` failed, we would use `Inputs.CompileCommand` which
should be the fallback command set in ClangdServer. It might be a good idea to
use the old command, but it doesn't seem to be in the scope of this patch.
Added a `FIXME`.
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
ilya-biryukov wrote:
> This is racy, `FileInputs` is only accessed from a worker thread now.
> I'm afraid we'll need a separate variable with a lock around it (could reuse
> one lock for preamble and compile command, probably)
It looks like we could also guard `FileInputs`? Adding another variable seems
to make the state more complicated.
Comment at: clangd/TUScheduler.cpp:835
void TUScheduler::update(PathRef File, ParseInputs Inputs,
WantDiagnostics WantDiags) {
ilya-biryukov wrote:
> We should add a comment that compile command in inputs is ignored.
> IMO even better is to accept an `FS` and `Contents` instead of `ParseInputs`.
Added commen. Unfortunately, `ParseInputs` contains other things like `Index`
and `Opts`, so we couldn't replace it with FS and Contents.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric updated this revision to Diff 194850.
ioeric marked 7 inline comments as done.
ioeric added a comment.
- address review comments
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
namespace clangd {
namespace {
-using ::testing::_;
-using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(getDefaultAsyncThreadsCount(),
+ TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -154,7 +152,7 @@
// thread until we've scheduled them all.
Notification Ready;
TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -184,7 +182,7 @@
TEST_F(TUSchedulerTests, Debounce) {
std::atomic CallbackCount(0);
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
@@ -220,7 +218,7 @@
{
Notification InconsistentReadDone; // Must live longest.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -277,7 +275,7 @@
{
Notification Proceed; // Ensure we schedule everything.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -346,7 +344,7 @@
// Run TUScheduler and collect some stats.
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -437,10 +435,11 @@
std::atomic BuiltASTCounter(0);
ASTRetentionPolicy Policy;
Policy.MaxRetainedASTs = 2;
- TUScheduler S(
- /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
llvm::StringLiteral SourceContents = R"cpp(
int* a;
@@ -487,11 +486,11 @@
}
TEST_F(TUSchedulerTests, EmptyPreamble) {
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
// Testing strategy: we update the file and schedule a few preamble reads at
// the same time. All reads should get the same non-null preamble.
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/tru
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric updated this revision to Diff 194851.
ioeric added a comment.
- Add missing comment to TUScheduler.h
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
namespace clangd {
namespace {
-using ::testing::_;
-using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(getDefaultAsyncThreadsCount(),
+ TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -154,7 +152,7 @@
// thread until we've scheduled them all.
Notification Ready;
TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -184,7 +182,7 @@
TEST_F(TUSchedulerTests, Debounce) {
std::atomic CallbackCount(0);
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
@@ -220,7 +218,7 @@
{
Notification InconsistentReadDone; // Must live longest.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -277,7 +275,7 @@
{
Notification Proceed; // Ensure we schedule everything.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -346,7 +344,7 @@
// Run TUScheduler and collect some stats.
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -437,10 +435,11 @@
std::atomic BuiltASTCounter(0);
ASTRetentionPolicy Policy;
Policy.MaxRetainedASTs = 2;
- TUScheduler S(
- /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
llvm::StringLiteral SourceContents = R"cpp(
int* a;
@@ -487,11 +486,11 @@
}
TEST_F(TUSchedulerTests, EmptyPreamble) {
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
// Testing strategy: we update the file and schedule a few preamble reads at
// the same time. All reads should get the same non-null preamble.
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCal
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ilya-biryukov added a comment.
This is long overdue
Comment at: clangd/TUScheduler.cpp:338
+ Barrier(Barrier), Done(false) {
+ FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}
The command is filled with a fallback by `ClangdServer`, right? Why do we need
to repeat this here?
Comment at: clangd/TUScheduler.cpp:358
+// current implementation.
+if (auto Cmd = CDB.getCompileCommand(FileName))
+ Inputs.CompileCommand = *Cmd;
Could you document that we initially start with a fallback, so update here?
Comment at: clangd/TUScheduler.cpp:359
+if (auto Cmd = CDB.getCompileCommand(FileName))
+ Inputs.CompileCommand = *Cmd;
// Will be used to check if we can avoid rebuilding the AST.
Maybe update the heuristic field in the current compile command if its empty to
indicate we're using an older command?
That might happen if a previous call to `getCompileCommand` succeeded (so the
stored command does not have a heuristic set).
Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand &ASTWorker::getCurrentCompileCommand() const {
+ return FileInputs.CompileCommand;
+}
This is racy, `FileInputs` is only accessed from a worker thread now.
I'm afraid we'll need a separate variable with a lock around it (could reuse
one lock for preamble and compile command, probably)
Comment at: clangd/TUScheduler.cpp:835
void TUScheduler::update(PathRef File, ParseInputs Inputs,
WantDiagnostics WantDiags) {
We should add a comment that compile command in inputs is ignored.
IMO even better is to accept an `FS` and `Contents` instead of `ParseInputs`.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60607/new/
https://reviews.llvm.org/D60607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer
ioeric created this revision.
ioeric added reviewers: sammccall, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
javed.absar.
Herald added a project: clang.
This makes addDocument non-blocking and would also allow code completion
(in fallback mode) to run when worker waits for the compile command.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D60607
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Compiler.h
clangd/GlobalCompilationDatabase.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
namespace clangd {
namespace {
-using ::testing::_;
-using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
TUSchedulerTests::DiagsCallbackKey;
TEST_F(TUSchedulerTests, MissingFiles) {
- TUScheduler S(getDefaultAsyncThreadsCount(),
+ TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -154,7 +152,7 @@
// thread until we've scheduled them all.
Notification Ready;
TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -184,7 +182,7 @@
TEST_F(TUSchedulerTests, Debounce) {
std::atomic CallbackCount(0);
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
@@ -220,7 +218,7 @@
{
Notification InconsistentReadDone; // Must live longest.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -277,7 +275,7 @@
{
Notification Proceed; // Ensure we schedule everything.
TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -346,7 +344,7 @@
// Run TUScheduler and collect some stats.
{
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -437,10 +435,11 @@
std::atomic BuiltASTCounter(0);
ASTRetentionPolicy Policy;
Policy.MaxRetainedASTs = 2;
- TUScheduler S(
- /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
llvm::StringLiteral SourceContents = R"cpp(
int* a;
@@ -487,11 +486,11 @@
}
TEST_F(TUSchedulerTests, EmptyPreamble) {
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
// Testing strategy: we update the file and schedule a few preamble reads at
// the same time. All reads should get the same non-null preamble.
- TUScheduler S(
- /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- /*ASTCallbacks=*/nullptr,
- /*UpdateDebounce=*/std::chrono::steady_clock:
