kadircet created this revision.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
javed.absar, ilya-biryukov.
Herald added a project: clang.
kadircet added a reviewer: sammccall.
This is an extension clangd provides in LSP layer and to embedders of
ClangdServer. Currently there are no users of WantDiagnostics::Yes apart from
our tests, as from a practical point of view it is quite similar to
WantDiagnostics::Auto.
This patch gets rid of WantDiagnostics::Yes to get rid of some logic in
TUScheduler that handles it.
Tests are updated using the facts that an update is executed if:
- there's a read following it, or
- it is the last update in the request queue.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D81456
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1118,7 +1118,7 @@
runAddDocument(Server, FooCpp, FooWithHeader.code());
// Both preamble and AST are built in this request.
Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
- WantDiagnostics::Yes);
+ WantDiagnostics::Auto);
// Use the AST being built in above request.
EXPECT_THAT(
cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -205,12 +205,14 @@
// To avoid a racy test, don't allow tasks to actually run on the worker
// thread until we've scheduled them all.
Notification Ready;
+ Notification ScheduledFirstUpdate;
TUScheduler S(CDB, optsForTest(), captureDiags());
auto Path = testPath("foo.cpp");
- updateWithDiags(S, Path, "", WantDiagnostics::Yes,
- [&](std::vector<Diag>) { Ready.wait(); });
- updateWithDiags(S, Path, "request diags", WantDiagnostics::Yes,
- [&](std::vector<Diag>) { ++CallbackCount; });
+ updateWithDiags(S, Path, "", WantDiagnostics::Auto, [&](std::vector<Diag>) {
+ ScheduledFirstUpdate.notify();
+ Ready.wait();
+ });
+ ScheduledFirstUpdate.wait();
updateWithDiags(S, Path, "auto (clobbered)", WantDiagnostics::Auto,
[&](std::vector<Diag>) {
ADD_FAILURE()
@@ -226,7 +228,7 @@
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
- EXPECT_EQ(2, CallbackCount);
+ EXPECT_EQ(1, CallbackCount);
}
TEST_F(TUSchedulerTests, Debounce) {
@@ -264,7 +266,7 @@
// R2B
// U3(WantDiags=Yes)
// R3 <-- cancelled
- std::vector<std::string> DiagsSeen, ReadsSeen, ReadsCanceled;
+ std::vector<std::string> DiagsSeen, ReadsSeen, ReadsCancelled;
{
Notification Proceed; // Ensure we schedule everything.
TUScheduler S(CDB, optsForTest(), captureDiags());
@@ -274,7 +276,7 @@
auto T = cancelableTask();
WithContext C(std::move(T.first));
updateWithDiags(
- S, Path, "//" + ID, WantDiagnostics::Yes,
+ S, Path, "//" + ID, WantDiagnostics::Auto,
[&, ID](std::vector<Diag> Diags) { DiagsSeen.push_back(ID); });
return std::move(T.second);
};
@@ -285,7 +287,7 @@
S.runWithAST(ID, Path, [&, ID](llvm::Expected<InputsAndAST> E) {
if (auto Err = E.takeError()) {
if (Err.isA<CancelledError>()) {
- ReadsCanceled.push_back(ID);
+ ReadsCancelled.push_back(ID);
consumeError(std::move(Err));
} else {
ADD_FAILURE() << "Non-cancelled error for " << ID << ": "
@@ -298,8 +300,12 @@
return std::move(T.second);
};
- updateWithCallback(S, Path, "", WantDiagnostics::Yes,
- [&]() { Proceed.wait(); });
+ Notification ScheduledUpdate;
+ updateWithCallback(S, Path, "", WantDiagnostics::Auto, [&]() {
+ ScheduledUpdate.notify();
+ Proceed.wait();
+ });
+ ScheduledUpdate.wait();
// The second parens indicate cancellation, where present.
Update("U1")();
Read("R1")();
@@ -314,11 +320,11 @@
}
EXPECT_THAT(DiagsSeen, ElementsAre("U2", "U3"))
<< "U1 and all dependent reads were cancelled. "
- "U2 has a dependent read R2A. "
+ "U2 has a dependent read R2B. "
"U3 was not cancelled.";
EXPECT_THAT(ReadsSeen, ElementsAre("R2B"))
<< "All reads other than R2B were cancelled";
- EXPECT_THAT(ReadsCanceled, ElementsAre("R1", "R2A", "R3"))
+ EXPECT_THAT(ReadsCancelled, ElementsAre("R1", "R2A", "R3"))
<< "All reads other than R2B were cancelled";
}
@@ -352,7 +358,7 @@
std::atomic<int> Builds(0), Actions(0);
Notification Start;
- updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
+ updateWithDiags(S, Path, "a", WantDiagnostics::Auto, [&](std::vector<Diag>) {
++Builds;
Start.wait();
});
@@ -544,7 +550,7 @@
EXPECT_THAT(Tracer.takeMetric("ast_access_diag", "miss"), SizeIs(0));
// Build one file in advance. We will not access it later, so it will be the
// one that the cache will evict.
- updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Yes,
+ updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Auto,
[&BuiltASTCounter]() { ++BuiltASTCounter; });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
ASSERT_EQ(BuiltASTCounter.load(), 1);
@@ -553,9 +559,9 @@
// Build two more files. Since we can retain only 2 ASTs, these should be
// the ones we see in the cache later.
- updateWithCallback(S, Bar, SourceContents, WantDiagnostics::Yes,
+ updateWithCallback(S, Bar, SourceContents, WantDiagnostics::Auto,
[&BuiltASTCounter]() { ++BuiltASTCounter; });
- updateWithCallback(S, Baz, SourceContents, WantDiagnostics::Yes,
+ updateWithCallback(S, Baz, SourceContents, WantDiagnostics::Auto,
[&BuiltASTCounter]() { ++BuiltASTCounter; });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
ASSERT_EQ(BuiltASTCounter.load(), 3);
@@ -566,7 +572,7 @@
ASSERT_THAT(S.getFilesWithCachedAST(), UnorderedElementsAre(Bar, Baz));
// Access the old file again.
- updateWithCallback(S, Foo, OtherSourceContents, WantDiagnostics::Yes,
+ updateWithCallback(S, Foo, OtherSourceContents, WantDiagnostics::Auto,
[&BuiltASTCounter]() { ++BuiltASTCounter; });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
ASSERT_EQ(BuiltASTCounter.load(), 4);
@@ -697,7 +703,7 @@
auto DoUpdate = [&](std::string Contents) -> bool {
std::atomic<bool> Updated(false);
Updated = false;
- updateWithDiags(S, Source, Contents, WantDiagnostics::Yes,
+ updateWithDiags(S, Source, Contents, WantDiagnostics::Auto,
[&Updated](std::vector<Diag>) { Updated = true; });
bool UpdateFinished = S.blockUntilIdle(timeoutSeconds(10));
if (!UpdateFinished)
@@ -761,7 +767,7 @@
// Update the source contents, which should trigger an initial build with
// the header file missing.
updateWithDiags(
- S, Source, Inputs, WantDiagnostics::Yes,
+ S, Source, Inputs, WantDiagnostics::Auto,
[&DiagCount](std::vector<Diag> Diags) {
++DiagCount;
EXPECT_THAT(Diags,
@@ -775,12 +781,11 @@
FSProvider.Timestamps[HeaderB] = time_t(1);
// The addition of the missing header file triggers a rebuild, no errors.
- updateWithDiags(
- S, Source, Inputs, WantDiagnostics::Yes,
- [&DiagCount](std::vector<Diag> Diags) {
- ++DiagCount;
- EXPECT_THAT(Diags, IsEmpty());
- });
+ updateWithDiags(S, Source, Inputs, WantDiagnostics::Auto,
+ [&DiagCount](std::vector<Diag> Diags) {
+ ++DiagCount;
+ EXPECT_THAT(Diags, IsEmpty());
+ });
// Ensure previous assertions are done before we touch the FS again.
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
@@ -789,21 +794,22 @@
FSProvider.Timestamps[HeaderA] = time_t(1);
// This isn't detected: we don't stat a/foo.h to validate the preamble.
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+ updateWithDiags(S, Source, Inputs, WantDiagnostics::Auto,
[&DiagCount](std::vector<Diag> Diags) {
++DiagCount;
ADD_FAILURE()
<< "Didn't expect new diagnostics when adding a/foo.h";
});
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
// Forcing the reload should should cause a rebuild.
Inputs.ForceRebuild = true;
- updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
- [&DiagCount](std::vector<Diag> Diags) {
- ++DiagCount;
- ElementsAre(Field(&Diag::Message,
- "use of undeclared identifier 'b'"));
- });
+ updateWithDiags(
+ S, Source, Inputs, WantDiagnostics::Auto,
+ [&DiagCount](std::vector<Diag> Diags) {
+ ++DiagCount;
+ ElementsAre(Field(&Diag::Message, "use of undeclared identifier 'b'"));
+ });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
EXPECT_EQ(DiagCount, 3U);
@@ -954,7 +960,7 @@
TUScheduler S(CDB, optsForTest(), captureDiags());
std::vector<Diag> Diagnostics;
updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
- WantDiagnostics::Yes, [&](std::vector<Diag> D) {
+ WantDiagnostics::Auto, [&](std::vector<Diag> D) {
Diagnostics = std::move(D);
Ready.notify();
});
@@ -978,7 +984,7 @@
TUScheduler S(CDB, optsForTest(), captureDiags());
std::vector<Diag> Diagnostics;
updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
- WantDiagnostics::Yes, [&](std::vector<Diag> D) {
+ WantDiagnostics::Auto, [&](std::vector<Diag> D) {
Diagnostics = std::move(D);
Ready.notify();
});
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -1554,7 +1554,7 @@
}
int a = fun^
)cpp");
- Server.addDocument(FooCpp, Source.code(), "null", WantDiagnostics::Yes);
+ Server.addDocument(FooCpp, Source.code(), "null");
// We need to wait for preamble to build.
ASSERT_TRUE(Server.blockUntilIdleForTest());
Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -46,7 +46,6 @@
/// Determines whether diagnostics should be generated for a file snapshot.
enum class WantDiagnostics {
- Yes, /// Diagnostics must be generated for this snapshot.
No, /// Diagnostics must not be generated for this snapshot.
Auto, /// Diagnostics must be generated for this snapshot or a subsequent one,
/// within a bounded amount of time.
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -35,8 +35,7 @@
// invalidated by an update request, a new build will be requested on
// PreambleThread. Since PreambleThread only receives requests for newer
// versions of the file, in case of multiple requests it will only build the
-// last one and skip requests in between. Unless client force requested
-// diagnostics(WantDiagnostics::Yes).
+// last one and skip requests in between.
//
// When a new preamble is built, a "golden" AST is immediately built from that
// version of the file. This ensures diagnostics get updated even if the queue
@@ -239,14 +238,7 @@
return;
}
{
- std::unique_lock<std::mutex> Lock(Mutex);
- // If NextReq was requested with WantDiagnostics::Yes we cannot just drop
- // that on the floor. Block until we start building it. This won't
- // dead-lock as we are blocking the caller thread, while builds continue
- // on preamble thread.
- ReqCV.wait(Lock, [this] {
- return !NextReq || NextReq->WantDiags != WantDiagnostics::Yes;
- });
+ std::lock_guard<std::mutex> Lock(Mutex);
NextReq = std::move(Req);
}
// Let the worker thread know there's a request, notify_one is safe as there
@@ -1077,9 +1069,6 @@
Requests.push_front(std::move(R));
return Deadline::zero();
}
- // Cancelled updates are downgraded to auto-diagnostics, and may be elided.
- if (I->UpdateType == WantDiagnostics::Yes)
- I->UpdateType = WantDiagnostics::Auto;
}
while (shouldSkipHeadLocked()) {
@@ -1089,11 +1078,12 @@
assert(!Requests.empty() && "skipped the whole queue");
// Some updates aren't dead yet, but never end up being used.
// e.g. the first keystroke is live until obsoleted by the second.
- // We debounce "maybe-unused" writes, sleeping in case they become dead.
- // But don't delay reads (including updates where diagnostics are needed).
- for (const auto &R : Requests)
- if (R.UpdateType == None || R.UpdateType == WantDiagnostics::Yes)
+ // We debounce "maybe-unused" writes, sleeping in case they become dead. But
+ // don't delay reads.
+ for (const auto &R : Requests) {
+ if (R.UpdateType == llvm::None)
return Deadline::zero();
+ }
// Front request needs to be debounced, so determine when we're ready.
Deadline D(Requests.front().AddTime + UpdateDebounce.compute(RebuildTimes));
return D;
@@ -1113,16 +1103,14 @@
return false;
// The other way an update can be live is if its diagnostics might be used.
switch (*UpdateType) {
- case WantDiagnostics::Yes:
- return false; // Always used.
case WantDiagnostics::No:
return true; // Always dead.
case WantDiagnostics::Auto:
// Used unless followed by an update that generates diagnostics.
- for (; Next != Requests.end(); ++Next)
- if (Next->UpdateType == WantDiagnostics::Yes ||
- Next->UpdateType == WantDiagnostics::Auto)
+ for (; Next != Requests.end(); ++Next) {
+ if (Next->UpdateType == WantDiagnostics::Auto)
return true; // Prefer later diagnostics.
+ }
return false;
}
llvm_unreachable("Unknown WantDiagnostics");
Index: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -685,8 +685,8 @@
/// The actual content changes.
std::vector<TextDocumentContentChangeEvent> contentChanges;
- /// Forces diagnostics to be generated, or to not be generated, for this
- /// version of the file. If not set, diagnostics are eventually consistent:
+ /// Can be used to disalbe diagnostic generation for this version of the file.
+ /// If not provided or set to true, diagnostics are eventually consistent:
/// either they will be provided for this version or some subsequent one.
/// This is a clangd extension.
llvm::Optional<bool> wantDiagnostics;
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -188,7 +188,7 @@
/// Remove \p File from list of tracked files, schedule a request to free
/// resources associated with it. Pending diagnostics for closed files may not
- /// be delivered, even if requested with WantDiags::Auto or WantDiags::Yes.
+ /// be delivered, even if requested with WantDiags::Auto.
/// An empty set of diagnostics will be delivered, with Version = "".
void removeDocument(PathRef File);
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -667,15 +667,14 @@
const std::string &Contents = Params.textDocument.text;
auto Version = DraftMgr.addDraft(File, Params.textDocument.version, Contents);
- Server->addDocument(File, Contents, encodeVersion(Version),
- WantDiagnostics::Yes);
+ Server->addDocument(File, Contents, encodeVersion(Version));
}
void ClangdLSPServer::onDocumentDidChange(
const DidChangeTextDocumentParams &Params) {
auto WantDiags = WantDiagnostics::Auto;
if (Params.wantDiagnostics.hasValue())
- WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes
+ WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Auto
: WantDiagnostics::No;
PathRef File = Params.textDocument.uri.file();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
