ilya-biryukov created this revision.
ilya-biryukov added a reviewer: ioeric.
Herald added subscribers: jfb, arphaman, jkorous, MaskRay, javed.absar.
After r338256, clangd stopped reporting diagnostics if WantDiags::No request
is followed by a WantDiags::Yes request but the AST can be reused.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50045
Files:
clangd/TUScheduler.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===================================================================
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -390,5 +390,38 @@
ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents)));
}
+TEST_F(TUSchedulerTests, NoChangeDiags) {
+ TUScheduler S(
+ /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
+ /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+ /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ ASTRetentionPolicy());
+
+ auto FooCpp = testPath("foo.cpp");
+ auto Contents = "int a; int b;";
+
+ S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::No,
+ [](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
+ S.runWithAST("touchAST", FooCpp, [](llvm::Expected<InputsAndAST> IA) {
+ // Make sure the AST was actually built.
+ cantFail(std::move(IA));
+ });
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+ // Even though the inputs didn't change and AST can be reused, we need to
+ // report the diagnostics, as they were not reported previously.
+ std::atomic<bool> SeenDiags(false);
+ S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto,
+ [&](std::vector<Diag>) { SeenDiags = true; });
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+ ASSERT_TRUE(SeenDiags);
+
+ // Subsequent request does not get any diagnostics callback because the same
+ // diags have previously been reported and the inputs didn't change.
+ S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto,
+ [&](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -228,6 +228,9 @@
Semaphore &Barrier;
/// Inputs, corresponding to the current state of AST.
ParseInputs FileInputs;
+ /// Whether the diagnostics for the current FileInputs were reported to the
+ /// users before.
+ bool DiagsWereReported = false;
/// Size of the last AST
/// Guards members used by both TUScheduler and the worker thread.
mutable std::mutex Mutex;
@@ -330,7 +333,9 @@
std::tie(Inputs.CompileCommand, Inputs.Contents);
tooling::CompileCommand OldCommand = std::move(FileInputs.CompileCommand);
+ bool PrevDiagsWereReported = DiagsWereReported;
FileInputs = Inputs;
+ DiagsWereReported = false;
log("Updating file {0} with command [{1}] {2}", FileName,
Inputs.CompileCommand.Directory,
@@ -366,7 +371,12 @@
OldPreamble.reset();
PreambleWasBuilt.notify();
- if (CanReuseAST) {
+ if (CanReuseAST)
+ DiagsWereReported = PrevDiagsWereReported;
+ else
+ IdleASTs.take(this); // Remove the old AST if it's still in cache.
+
+ if (DiagsWereReported) {
// Take a shortcut and don't build the AST if neither the inputs nor the
// preamble have changed.
// Note that we do not report the diagnostics, since they should not have
@@ -380,20 +390,24 @@
FileName);
return;
}
- // Remove the old AST if it's still in cache.
- IdleASTs.take(this);
// Build the AST for diagnostics.
- llvm::Optional<ParsedAST> AST =
- buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
+ llvm::Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
+ if (!AST) {
+ llvm::Optional<ParsedAST> NewAST =
+ buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
+ AST = NewAST ? llvm::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
+ }
+
// We want to report the diagnostics even if this update was cancelled.
// It seems more useful than making the clients wait indefinitely if they
// spam us with updates.
- if (WantDiags != WantDiagnostics::No && AST)
- OnUpdated(AST->getDiagnostics());
+ if (WantDiags != WantDiagnostics::No && *AST) {
+ OnUpdated((*AST)->getDiagnostics());
+ DiagsWereReported = true;
+ }
// Stash the AST in the cache for further use.
- IdleASTs.put(this,
- AST ? llvm::make_unique<ParsedAST>(std::move(*AST)) : nullptr);
+ IdleASTs.put(this, std::move(*AST));
};
startTask("Update", Bind(Task, std::move(OnUpdated)), WantDiags);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
