[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
sammccall updated this revision to Diff 239652.
sammccall added a comment.
Revert changes to VSCode client. This experimental version of the VSCode libs
is fairly new and some corp mirrors we care about are behind ;-)
also clang-format
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
Files:
clang-tools-extra/clangd/Cancellation.cpp
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/index/Background.cpp
clang-tools-extra/clangd/index/Background.h
clang-tools-extra/clangd/index/BackgroundQueue.cpp
clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
clang-tools-extra/clangd/test/background-index.test
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
llvm/include/llvm/Support/JSON.h
Index: llvm/include/llvm/Support/JSON.h
===
--- llvm/include/llvm/Support/JSON.h
+++ llvm/include/llvm/Support/JSON.h
@@ -598,6 +598,13 @@
}
return false;
}
+inline bool fromJSON(const Value &E, std::nullptr_t &Out) {
+ if (auto S = E.getAsNull()) {
+Out = *S;
+return true;
+ }
+ return false;
+}
template bool fromJSON(const Value &E, llvm::Optional &Out) {
if (E.getAsNull()) {
Out = llvm::None;
Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -711,5 +711,53 @@
}
}
+TEST(BackgroundQueueTest, Progress) {
+ using testing::AnyOf;
+ BackgroundQueue::Stats S;
+ BackgroundQueue Q([&](BackgroundQueue::Stats New) {
+// Verify values are sane.
+// Items are enqueued one at a time (at least in this test).
+EXPECT_THAT(New.Enqueued, AnyOf(S.Enqueued, S.Enqueued + 1));
+// Items are completed one at a time.
+EXPECT_THAT(New.Completed, AnyOf(S.Completed, S.Completed + 1));
+// Items are started or completed one at a time.
+EXPECT_THAT(New.Active, AnyOf(S.Active - 1, S.Active, S.Active + 1));
+// Idle point only advances in time.
+EXPECT_GE(New.LastIdle, S.LastIdle);
+// Idle point is a task that has been completed in the past.
+EXPECT_LE(New.LastIdle, New.Completed);
+// LastIdle is now only if we're really idle.
+EXPECT_EQ(New.LastIdle == New.Enqueued,
+ New.Completed == New.Enqueued && New.Active == 0u);
+S = New;
+ });
+
+ // Two types of tasks: a ping task enqueues a pong task.
+ // This avoids all enqueues followed by all completions (boring!)
+ std::atomic PingCount(0), PongCount(0);
+ BackgroundQueue::Task Pong([&] { ++PongCount; });
+ BackgroundQueue::Task Ping([&] {
+++PingCount;
+Q.push(Pong);
+ });
+
+ for (int I = 0; I < 1000; ++I)
+Q.push(Ping);
+ // Spin up some workers and stop while idle.
+ AsyncTaskRunner ThreadPool;
+ for (unsigned I = 0; I < 5; ++I)
+ThreadPool.runAsync("worker", [&] { Q.work([&] { Q.stop(); }); });
+ ThreadPool.wait();
+
+ // Everything's done, check final stats.
+ // Assertions above ensure we got from 0 to 2000 in a reasonable way.
+ EXPECT_EQ(PingCount.load(), 1000);
+ EXPECT_EQ(PongCount.load(), 1000);
+ EXPECT_EQ(S.Active, 0u);
+ EXPECT_EQ(S.Enqueued, 2000u);
+ EXPECT_EQ(S.Completed, 2000u);
+ EXPECT_EQ(S.LastIdle, 2000u);
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/background-index.test
===
--- clang-tools-extra/clangd/test/background-index.test
+++ clang-tools-extra/clangd/test/background-index.test
@@ -11,7 +11,8 @@
# We're editing bar.cpp, which includes foo.h.
# foo() is declared in foo.h and defined in foo.cpp.
# The background index should allow us to go-to-definition on foo().
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# We should also see indexing progress notifications.
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,BUILD
# Test that the index is writing files in the expected location.
# RUN: ls %t/.clangd/index/foo.cpp.*.idx
@@ -19,4 +20,4 @@
# Test the index is read from disk: delete code and restart clangd.
# RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,USE
Index: clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
sammccall created this revision.
sammccall added reviewers: kadircet, usaxena95.
Herald added subscribers: llvm-commits, cfe-commits, jfb, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added projects: clang, LLVM.
sammccall updated this revision to Diff 239652.
sammccall added a comment.
sammccall updated this revision to Diff 239653.
Revert changes to VSCode client. This experimental version of the VSCode libs
is fairly new and some corp mirrors we care about are behind ;-)
also clang-format
sammccall added a comment.
revert accidental change
It simply shows the completed/total items on the background queue, e.g.
indexing: 233/1000
The denominator is reset to zero every time the queue goes idle.
The protocol is fairly complicated here (requires creating a remote "progress"
resource before sending updates). We implement the full protocol, but I've added
an extension allowing it to be skipped to reduce the burden on clients - in
particular the lit test takes this shortcut.
The addition of background index progress to DiagnosticConsumer seems ridiculous
at first glance, but I believe that interface is trending in the direction of
"ClangdServer callbacks" anyway. It's due for a rename, but otherwise actually
fits.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D73218
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/index/Background.cpp
clang-tools-extra/clangd/index/Background.h
clang-tools-extra/clangd/index/BackgroundQueue.cpp
clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
clang-tools-extra/clangd/test/background-index.test
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
llvm/include/llvm/Support/JSON.h
Index: llvm/include/llvm/Support/JSON.h
===
--- llvm/include/llvm/Support/JSON.h
+++ llvm/include/llvm/Support/JSON.h
@@ -598,6 +598,13 @@
}
return false;
}
+inline bool fromJSON(const Value &E, std::nullptr_t &Out) {
+ if (auto S = E.getAsNull()) {
+Out = *S;
+return true;
+ }
+ return false;
+}
template bool fromJSON(const Value &E, llvm::Optional &Out) {
if (E.getAsNull()) {
Out = llvm::None;
Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -711,5 +711,53 @@
}
}
+TEST(BackgroundQueueTest, Progress) {
+ using testing::AnyOf;
+ BackgroundQueue::Stats S;
+ BackgroundQueue Q([&](BackgroundQueue::Stats New) {
+// Verify values are sane.
+// Items are enqueued one at a time (at least in this test).
+EXPECT_THAT(New.Enqueued, AnyOf(S.Enqueued, S.Enqueued + 1));
+// Items are completed one at a time.
+EXPECT_THAT(New.Completed, AnyOf(S.Completed, S.Completed + 1));
+// Items are started or completed one at a time.
+EXPECT_THAT(New.Active, AnyOf(S.Active - 1, S.Active, S.Active + 1));
+// Idle point only advances in time.
+EXPECT_GE(New.LastIdle, S.LastIdle);
+// Idle point is a task that has been completed in the past.
+EXPECT_LE(New.LastIdle, New.Completed);
+// LastIdle is now only if we're really idle.
+EXPECT_EQ(New.LastIdle == New.Enqueued,
+ New.Completed == New.Enqueued && New.Active == 0u);
+S = New;
+ });
+
+ // Two types of tasks: a ping task enqueues a pong task.
+ // This avoids all enqueues followed by all completions (boring!)
+ std::atomic PingCount(0), PongCount(0);
+ BackgroundQueue::Task Pong([&] { ++PongCount; });
+ BackgroundQueue::Task Ping([&] {
+++PingCount;
+Q.push(Pong);
+ });
+
+ for (int I = 0; I < 1000; ++I)
+Q.push(Ping);
+ // Spin up some workers and stop while idle.
+ AsyncTaskRunner ThreadPool;
+ for (unsigned I = 0; I < 5; ++I)
+ThreadPool.runAsync("worker", [&] { Q.work([&] { Q.stop(); }); });
+ ThreadPool.wait();
+
+ // Everything's done, check final stats.
+ // Assertions above ensure we got from 0 to 2000 in a reasonable way.
+ EXPECT_EQ(PingCount.load(), 1000);
+ EXPECT_EQ(PongCount.load(), 1000);
+ EXPECT_EQ(S.Active, 0u);
+ EXPECT_EQ(S.Enqueued, 2000u);
+ EXPECT_EQ(S.Completed, 2000u);
+ EXPECT_EQ(S.LastIdle, 2000u);
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/background-index.test
===
--- clang-tools-extra/clangd/test/background-index.test
+++ clang-tools-extra/clangd/test/background-index.test
@@ -11,7 +11,8 @@
# We're editing bar.cpp, which includes foo.h.
# foo() is declared in foo.h and defined in foo.cpp.
# The background in
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
sammccall updated this revision to Diff 239653.
sammccall added a comment.
revert accidental change
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/index/Background.cpp
clang-tools-extra/clangd/index/Background.h
clang-tools-extra/clangd/index/BackgroundQueue.cpp
clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
clang-tools-extra/clangd/test/background-index.test
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
llvm/include/llvm/Support/JSON.h
Index: llvm/include/llvm/Support/JSON.h
===
--- llvm/include/llvm/Support/JSON.h
+++ llvm/include/llvm/Support/JSON.h
@@ -598,6 +598,13 @@
}
return false;
}
+inline bool fromJSON(const Value &E, std::nullptr_t &Out) {
+ if (auto S = E.getAsNull()) {
+Out = *S;
+return true;
+ }
+ return false;
+}
template bool fromJSON(const Value &E, llvm::Optional &Out) {
if (E.getAsNull()) {
Out = llvm::None;
Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -711,5 +711,53 @@
}
}
+TEST(BackgroundQueueTest, Progress) {
+ using testing::AnyOf;
+ BackgroundQueue::Stats S;
+ BackgroundQueue Q([&](BackgroundQueue::Stats New) {
+// Verify values are sane.
+// Items are enqueued one at a time (at least in this test).
+EXPECT_THAT(New.Enqueued, AnyOf(S.Enqueued, S.Enqueued + 1));
+// Items are completed one at a time.
+EXPECT_THAT(New.Completed, AnyOf(S.Completed, S.Completed + 1));
+// Items are started or completed one at a time.
+EXPECT_THAT(New.Active, AnyOf(S.Active - 1, S.Active, S.Active + 1));
+// Idle point only advances in time.
+EXPECT_GE(New.LastIdle, S.LastIdle);
+// Idle point is a task that has been completed in the past.
+EXPECT_LE(New.LastIdle, New.Completed);
+// LastIdle is now only if we're really idle.
+EXPECT_EQ(New.LastIdle == New.Enqueued,
+ New.Completed == New.Enqueued && New.Active == 0u);
+S = New;
+ });
+
+ // Two types of tasks: a ping task enqueues a pong task.
+ // This avoids all enqueues followed by all completions (boring!)
+ std::atomic PingCount(0), PongCount(0);
+ BackgroundQueue::Task Pong([&] { ++PongCount; });
+ BackgroundQueue::Task Ping([&] {
+++PingCount;
+Q.push(Pong);
+ });
+
+ for (int I = 0; I < 1000; ++I)
+Q.push(Ping);
+ // Spin up some workers and stop while idle.
+ AsyncTaskRunner ThreadPool;
+ for (unsigned I = 0; I < 5; ++I)
+ThreadPool.runAsync("worker", [&] { Q.work([&] { Q.stop(); }); });
+ ThreadPool.wait();
+
+ // Everything's done, check final stats.
+ // Assertions above ensure we got from 0 to 2000 in a reasonable way.
+ EXPECT_EQ(PingCount.load(), 1000);
+ EXPECT_EQ(PongCount.load(), 1000);
+ EXPECT_EQ(S.Active, 0u);
+ EXPECT_EQ(S.Enqueued, 2000u);
+ EXPECT_EQ(S.Completed, 2000u);
+ EXPECT_EQ(S.LastIdle, 2000u);
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/background-index.test
===
--- clang-tools-extra/clangd/test/background-index.test
+++ clang-tools-extra/clangd/test/background-index.test
@@ -11,7 +11,8 @@
# We're editing bar.cpp, which includes foo.h.
# foo() is declared in foo.h and defined in foo.cpp.
# The background index should allow us to go-to-definition on foo().
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# We should also see indexing progress notifications.
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,BUILD
# Test that the index is writing files in the expected location.
# RUN: ls %t/.clangd/index/foo.cpp.*.idx
@@ -19,4 +20,4 @@
# Test the index is read from disk: delete code and restart clangd.
# RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,USE
Index: clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
===
--- clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
+++ clang-tools-extra/clangd/test/Inputs/background-index/definition
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
merge_guards_bot added a comment.
{icon question-circle color=gray} Unit tests: unknown.
{icon times-circle color=red} clang-tidy: fail. clang-tidy found 1 errors and
11 warnings
.
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
merge_guards_bot added a comment.
{icon question-circle color=gray} Unit tests: unknown.
{icon times-circle color=red} clang-tidy: fail. clang-tidy found 1 errors and
11 warnings
.
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
merge_guards_bot added a comment.
{icon question-circle color=gray} Unit tests: unknown.
{icon times-circle color=red} clang-tidy: fail. clang-tidy found 1 errors and
11 warnings
.
{icon times-circle color=red} clang-format: fail. Please format your changes
with clang-format by running `git-clang-format HEAD^` or applying this patch
.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:223
+// We've requested the client to create a progress bar.
+// Meanwhile, the state is buffered in PendingBackgraundIndexProgress.
+Creating,
s/PendingBackgraundIndexProgress/PendingBackgroundIndexProgress/
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:232
+ /// LSP extension: skip WorkDoneProgressCreate, just send progress streams.
+ bool BackgroundIndexSkipCreate;
// Store of the current versions of the open documents.
initialize to `false`
Comment at: clang-tools-extra/clangd/Protocol.cpp:383
+
+llvm::json::Value toJSON(const WorkDoneProgressBegin &P) {
+ llvm::json::Object Result{
why not have a single struct with that has a required `kind` field and a bunch
of optional fields.
Later on we can assert on the existence of fields depending on the kind, I
think it would simplify the implementation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
sammccall updated this revision to Diff 240121.
sammccall marked 3 inline comments as done.
sammccall added a comment.
address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/index/Background.cpp
clang-tools-extra/clangd/index/Background.h
clang-tools-extra/clangd/index/BackgroundQueue.cpp
clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
clang-tools-extra/clangd/test/background-index.test
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
llvm/include/llvm/Support/JSON.h
Index: llvm/include/llvm/Support/JSON.h
===
--- llvm/include/llvm/Support/JSON.h
+++ llvm/include/llvm/Support/JSON.h
@@ -598,6 +598,13 @@
}
return false;
}
+inline bool fromJSON(const Value &E, std::nullptr_t &Out) {
+ if (auto S = E.getAsNull()) {
+Out = *S;
+return true;
+ }
+ return false;
+}
template bool fromJSON(const Value &E, llvm::Optional &Out) {
if (E.getAsNull()) {
Out = llvm::None;
Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -711,5 +711,53 @@
}
}
+TEST(BackgroundQueueTest, Progress) {
+ using testing::AnyOf;
+ BackgroundQueue::Stats S;
+ BackgroundQueue Q([&](BackgroundQueue::Stats New) {
+// Verify values are sane.
+// Items are enqueued one at a time (at least in this test).
+EXPECT_THAT(New.Enqueued, AnyOf(S.Enqueued, S.Enqueued + 1));
+// Items are completed one at a time.
+EXPECT_THAT(New.Completed, AnyOf(S.Completed, S.Completed + 1));
+// Items are started or completed one at a time.
+EXPECT_THAT(New.Active, AnyOf(S.Active - 1, S.Active, S.Active + 1));
+// Idle point only advances in time.
+EXPECT_GE(New.LastIdle, S.LastIdle);
+// Idle point is a task that has been completed in the past.
+EXPECT_LE(New.LastIdle, New.Completed);
+// LastIdle is now only if we're really idle.
+EXPECT_EQ(New.LastIdle == New.Enqueued,
+ New.Completed == New.Enqueued && New.Active == 0u);
+S = New;
+ });
+
+ // Two types of tasks: a ping task enqueues a pong task.
+ // This avoids all enqueues followed by all completions (boring!)
+ std::atomic PingCount(0), PongCount(0);
+ BackgroundQueue::Task Pong([&] { ++PongCount; });
+ BackgroundQueue::Task Ping([&] {
+++PingCount;
+Q.push(Pong);
+ });
+
+ for (int I = 0; I < 1000; ++I)
+Q.push(Ping);
+ // Spin up some workers and stop while idle.
+ AsyncTaskRunner ThreadPool;
+ for (unsigned I = 0; I < 5; ++I)
+ThreadPool.runAsync("worker", [&] { Q.work([&] { Q.stop(); }); });
+ ThreadPool.wait();
+
+ // Everything's done, check final stats.
+ // Assertions above ensure we got from 0 to 2000 in a reasonable way.
+ EXPECT_EQ(PingCount.load(), 1000);
+ EXPECT_EQ(PongCount.load(), 1000);
+ EXPECT_EQ(S.Active, 0u);
+ EXPECT_EQ(S.Enqueued, 2000u);
+ EXPECT_EQ(S.Completed, 2000u);
+ EXPECT_EQ(S.LastIdle, 2000u);
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/background-index.test
===
--- clang-tools-extra/clangd/test/background-index.test
+++ clang-tools-extra/clangd/test/background-index.test
@@ -11,7 +11,8 @@
# We're editing bar.cpp, which includes foo.h.
# foo() is declared in foo.h and defined in foo.cpp.
# The background index should allow us to go-to-definition on foo().
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# We should also see indexing progress notifications.
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,BUILD
# Test that the index is writing files in the expected location.
# RUN: ls %t/.clangd/index/foo.cpp.*.idx
@@ -19,4 +20,4 @@
# Test the index is read from disk: delete code and restart clangd.
# RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,USE
Index: clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
===
--- clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
+++ clang-tools-extra/clangd/tes
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/Protocol.cpp:383
+
+llvm::json::Value toJSON(const WorkDoneProgressBegin &P) {
+ llvm::json::Object Result{
kadircet wrote:
> why not have a single struct with that has a required `kind` field and a
> bunch of optional fields.
> Later on we can assert on the existence of fields depending on the kind, I
> think it would simplify the implementation.
Yeah, this was my first thought/attempt. It's possible but I think not ideal in
the end.
The main downsides:
- this would diverge from LSP and so make it harder to cross-reference things
- this would diverge from LSP and usually we don't, so it's surprising
- the semantics of the "same" field across different events are sometimes
subtly different. e.g. Begin.cancellable is whether a cancel button is shown
(and in VSCode, the overall UI!) whereas End.cancellable controls its
enablement.
- between replicating the LSP docs, documenting divergence, documenting varied
optionality, and documenting varying semantics, the merged struct is really
hard to document well
- it's a bit harder to see the structure of the messages clangd sends this
way: e.g. we always send message in report, but never in begin/end, how we
handle percentage etc
Mostly I think following LSP closely is a good thing, but it sucks when the
spec is a mess :-(
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
merge_guards_bot added a comment.
{icon question-circle color=gray} Unit tests: unknown.
{icon times-circle color=red} clang-tidy: fail. clang-tidy found 8 errors and
12 warnings
.
12 of them are added as review comments below (why?
).
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
//Pre-merge checks is in beta. Report issue
.
Please join beta .//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/Protocol.cpp:383
+
+llvm::json::Value toJSON(const WorkDoneProgressBegin &P) {
+ llvm::json::Object Result{
sammccall wrote:
> kadircet wrote:
> > why not have a single struct with that has a required `kind` field and a
> > bunch of optional fields.
> > Later on we can assert on the existence of fields depending on the kind, I
> > think it would simplify the implementation.
> Yeah, this was my first thought/attempt. It's possible but I think not ideal
> in the end.
>
> The main downsides:
> - this would diverge from LSP and so make it harder to cross-reference things
> - this would diverge from LSP and usually we don't, so it's surprising
> - the semantics of the "same" field across different events are sometimes
> subtly different. e.g. Begin.cancellable is whether a cancel button is shown
> (and in VSCode, the overall UI!) whereas End.cancellable controls its
> enablement.
> - between replicating the LSP docs, documenting divergence, documenting
> varied optionality, and documenting varying semantics, the merged struct is
> really hard to document well
> - it's a bit harder to see the structure of the messages clangd sends this
> way: e.g. we always send message in report, but never in begin/end, how we
> handle percentage etc
>
> Mostly I think following LSP closely is a good thing, but it sucks when the
> spec is a mess :-(
> this would diverge from LSP and so make it harder to cross-reference things
ah sorry, I didn't know LSP defined those in 3 different structs, that makes
sense.
SG then
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7d20e80225b3: [clangd] Show background index status using
LSP 3.15 work-done progress… (authored by sammccall).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/index/Background.cpp
clang-tools-extra/clangd/index/Background.h
clang-tools-extra/clangd/index/BackgroundQueue.cpp
clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
clang-tools-extra/clangd/test/background-index.test
clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
llvm/include/llvm/Support/JSON.h
Index: llvm/include/llvm/Support/JSON.h
===
--- llvm/include/llvm/Support/JSON.h
+++ llvm/include/llvm/Support/JSON.h
@@ -598,6 +598,13 @@
}
return false;
}
+inline bool fromJSON(const Value &E, std::nullptr_t &Out) {
+ if (auto S = E.getAsNull()) {
+Out = *S;
+return true;
+ }
+ return false;
+}
template bool fromJSON(const Value &E, llvm::Optional &Out) {
if (E.getAsNull()) {
Out = llvm::None;
Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -711,5 +711,53 @@
}
}
+TEST(BackgroundQueueTest, Progress) {
+ using testing::AnyOf;
+ BackgroundQueue::Stats S;
+ BackgroundQueue Q([&](BackgroundQueue::Stats New) {
+// Verify values are sane.
+// Items are enqueued one at a time (at least in this test).
+EXPECT_THAT(New.Enqueued, AnyOf(S.Enqueued, S.Enqueued + 1));
+// Items are completed one at a time.
+EXPECT_THAT(New.Completed, AnyOf(S.Completed, S.Completed + 1));
+// Items are started or completed one at a time.
+EXPECT_THAT(New.Active, AnyOf(S.Active - 1, S.Active, S.Active + 1));
+// Idle point only advances in time.
+EXPECT_GE(New.LastIdle, S.LastIdle);
+// Idle point is a task that has been completed in the past.
+EXPECT_LE(New.LastIdle, New.Completed);
+// LastIdle is now only if we're really idle.
+EXPECT_EQ(New.LastIdle == New.Enqueued,
+ New.Completed == New.Enqueued && New.Active == 0u);
+S = New;
+ });
+
+ // Two types of tasks: a ping task enqueues a pong task.
+ // This avoids all enqueues followed by all completions (boring!)
+ std::atomic PingCount(0), PongCount(0);
+ BackgroundQueue::Task Pong([&] { ++PongCount; });
+ BackgroundQueue::Task Ping([&] {
+++PingCount;
+Q.push(Pong);
+ });
+
+ for (int I = 0; I < 1000; ++I)
+Q.push(Ping);
+ // Spin up some workers and stop while idle.
+ AsyncTaskRunner ThreadPool;
+ for (unsigned I = 0; I < 5; ++I)
+ThreadPool.runAsync("worker", [&] { Q.work([&] { Q.stop(); }); });
+ ThreadPool.wait();
+
+ // Everything's done, check final stats.
+ // Assertions above ensure we got from 0 to 2000 in a reasonable way.
+ EXPECT_EQ(PingCount.load(), 1000);
+ EXPECT_EQ(PongCount.load(), 1000);
+ EXPECT_EQ(S.Active, 0u);
+ EXPECT_EQ(S.Enqueued, 2000u);
+ EXPECT_EQ(S.Completed, 2000u);
+ EXPECT_EQ(S.LastIdle, 2000u);
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/background-index.test
===
--- clang-tools-extra/clangd/test/background-index.test
+++ clang-tools-extra/clangd/test/background-index.test
@@ -11,7 +11,8 @@
# We're editing bar.cpp, which includes foo.h.
# foo() is declared in foo.h and defined in foo.cpp.
# The background index should allow us to go-to-definition on foo().
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# We should also see indexing progress notifications.
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,BUILD
# Test that the index is writing files in the expected location.
# RUN: ls %t/.clangd/index/foo.cpp.*.idx
@@ -19,4 +20,4 @@
# Test the index is read from disk: delete code and restart clangd.
# RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc --check-prefixes=CHECK,USE
Index: clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
===
--- clang-tools-extra/clangd/test/Inputs/
