[clang-tools-extra] r338361 - [clangd] Report diagnostics even if WantDiags::No AST was reused
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 31 04:47:52 PDT 2018
Author: ibiryukov
Date: Tue Jul 31 04:47:52 2018
New Revision: 338361
URL: http://llvm.org/viewvc/llvm-project?rev=338361&view=rev
Log:
[clangd] Report diagnostics even if WantDiags::No AST was reused
Summary:
After r338256, clangd stopped reporting diagnostics if WantDiags::No request
is followed by a WantDiags::Yes request but the AST can be reused.
Reviewers: ioeric
Reviewed By: ioeric
Subscribers: javed.absar, MaskRay, jkorous, arphaman, jfb, cfe-commits
Differential Revision: https://reviews.llvm.org/D50045
Modified:
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=338361&r1=338360&r2=338361&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Tue Jul 31 04:47:52 2018
@@ -228,6 +228,9 @@ private:
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 @@ void ASTWorker::update(
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,34 +371,44 @@ void ASTWorker::update(
OldPreamble.reset();
PreambleWasBuilt.notify();
- if (CanReuseAST) {
- // 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
- // changed either. All the clients should handle the lack of OnUpdated()
- // call anyway to handle empty result from buildAST.
- // FIXME(ibiryukov): the AST could actually change if non-preamble
- // includes changed, but we choose to ignore it.
- // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the
- // current file at this point?
- log("Skipping rebuild of the AST for {0}, inputs are the same.",
- FileName);
- return;
+ if (!CanReuseAST) {
+ IdleASTs.take(this); // Remove the old AST if it's still in cache.
+ } else {
+ // Since we don't need to rebuild the AST, we might've already reported
+ // the diagnostics for it.
+ if (PrevDiagsWereReported) {
+ DiagsWereReported = true;
+ // Take a shortcut and don't report the diagnostics, since they should
+ // not changed. All the clients should handle the lack of OnUpdated()
+ // call anyway to handle empty result from buildAST.
+ // FIXME(ibiryukov): the AST could actually change if non-preamble
+ // includes changed, but we choose to ignore it.
+ // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the
+ // current file at this point?
+ log("Skipping rebuild of the AST for {0}, inputs are the same.",
+ FileName);
+ return;
+ }
+ }
+
+ // Get the AST for diagnostics.
+ 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;
}
- // 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);
// 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());
+ // Note *AST can be still be null if buildAST fails.
+ 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);
Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=338361&r1=338360&r2=338361&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Tue Jul 31 04:47:52 2018
@@ -390,5 +390,39 @@ TEST_F(TUSchedulerTests, NoopOnEmptyChan
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
More information about the cfe-commits
mailing list