hokein updated this revision to Diff 176098.
hokein marked 11 inline comments as done.
hokein added a comment.
Fix nits.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54796/new/
https://reviews.llvm.org/D54796
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===================================================================
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -7,12 +7,13 @@
//
//===----------------------------------------------------------------------===//
+#include "Annotations.h"
#include "Context.h"
#include "Matchers.h"
#include "TUScheduler.h"
#include "TestFS.h"
-#include "llvm/ADT/ScopeExit.h"
#include "gmock/gmock.h"
+#include "llvm/ADT/ScopeExit.h"
#include "gtest/gtest.h"
#include <algorithm>
#include <utility>
@@ -23,6 +24,7 @@
namespace {
using ::testing::_;
+using ::testing::AllOf;
using ::testing::AnyOf;
using ::testing::Each;
using ::testing::ElementsAre;
@@ -30,6 +32,9 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
+MATCHER_P(TUState, State, "") { return arg.Action.S == State; }
+MATCHER_P(ActionName, Name, "") { return arg.Action.Name == Name; }
+
class TUSchedulerTests : public ::testing::Test {
protected:
ParseInputs getInputs(PathRef File, std::string Contents) {
@@ -658,6 +663,78 @@
EXPECT_EQ(Counter.load(), 3);
}
+TEST_F(TUSchedulerTests, TUStatus) {
+ class CaptureTUStatus : public DiagnosticsConsumer {
+ public:
+ void onDiagnosticsReady(PathRef File,
+ std::vector<Diag> Diagnostics) override {}
+
+ void onFileUpdated(PathRef File, const TUStatus &Status) override {
+ std::lock_guard<std::mutex> Lock(Mutex);
+ AllStatus.push_back(Status);
+ }
+
+ std::vector<TUStatus> AllStatus;
+
+ private:
+ std::mutex Mutex;
+ } CaptureTUStatus;
+ MockFSProvider FS;
+ MockCompilationDatabase CDB;
+ ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
+ Annotations Code("int m^ain () {}");
+
+ // We schedule the following tasks in the queue:
+ // [Update] [GoToDefinition]
+ Server.addDocument(testPath("foo.cpp"), Code.code(), WantDiagnostics::Yes);
+ Server.findDefinitions(testPath("foo.cpp"), Code.point(),
+ [](Expected<std::vector<Location>> Result) {
+ ASSERT_TRUE((bool)Result);
+ });
+
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+ EXPECT_THAT(
+ CaptureTUStatus.AllStatus,
+ ElementsAre(
+ // Statuses of "Update" action.
+ TUState(TUAction::Queued),
+ AllOf(TUState(TUAction::RunningAction), ActionName("Update")),
+ TUState(TUAction::BuildingPreamble), TUState(TUAction::BuildingFile),
+
+ // Statuses of "Definitions" action
+ AllOf(TUState(TUAction::RunningAction), ActionName("Definitions")),
+ TUState(TUAction::Idle)));
+}
+
+TEST_F(TUSchedulerTests, NoTUStatusEmittedForRemovedFile) {
+ class CaptureTUStatus : public DiagnosticsConsumer {
+ public:
+ void onDiagnosticsReady(PathRef File,
+ std::vector<Diag> Diagnostics) override {}
+
+ void onFileUpdated(PathRef File, const TUStatus &Status) override {
+ ASSERT_TRUE(FirstRequest)
+ << "Expected to see exacly one file status updated.";
+ if (FirstRequest) {
+ ASSERT_TRUE(Status.Action.S == TUAction::Queued);
+ FirstRequest = false;
+ // We wait long enough that the document gets removed.
+ std::this_thread::sleep_for(std::chrono::milliseconds(50));
+ }
+ }
+ std::atomic<bool> FirstRequest = {true};
+ } CaptureTUStatus;
+ MockFSProvider FS;
+ MockCompilationDatabase CDB;
+ ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
+ Server.addDocument(testPath("foo.cpp"), "int main() {}",
+ WantDiagnostics::Yes);
+ Server.removeDocument(testPath("foo.cpp"));
+ ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for finishing";
+ ASSERT_TRUE(!CaptureTUStatus.FirstRequest);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/TUScheduler.h
===================================================================
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -52,6 +52,37 @@
unsigned MaxRetainedASTs = 3;
};
+struct TUAction {
+ enum State {
+ Unknown,
+ Queued, // The TU is pending in the thread task queue to be built.
+ RunningAction, // Starting running actions on the TU.
+ BuildingPreamble, // The preamble of the TU is being built.
+ BuildingFile, // The TU is being built.
+ Idle, // Indicates the worker thread is idle, and ready to run any upcoming
+ // actions.
+ };
+ TUAction() = default;
+ TUAction(State S, llvm::StringRef Name) : S(S), Name(Name) {};
+ State S = Unknown;
+ /// The name of the action currently running, e.g. Update, GoToDef, Hover.
+ /// Empty if we are in the idle state.
+ std::string Name;
+};
+
+// Internal status of the TU in TUScheduler.
+struct TUStatus {
+ struct BuildDetails {
+ /// indicates whether clang failed to build the TU.
+ bool BuildFailed = false;
+ /// indicates whether we reused the prebuilt AST.
+ bool ReuseAST = false;
+ };
+
+ TUAction Action;
+ BuildDetails Details;
+};
+
class ParsingCallbacks {
public:
virtual ~ParsingCallbacks() = default;
@@ -75,6 +106,9 @@
/// Called whenever the diagnostics for \p File are produced.
virtual void onDiagnostics(PathRef File, std::vector<Diag> Diags) {}
+
+ /// Called whenever the TU status is updated.
+ virtual void onFileUpdated(PathRef File, const TUStatus &Status) {}
};
/// Handles running tasks for ClangdServer and managing the resources (e.g.,
Index: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -205,6 +205,10 @@
/// Adds a new task to the end of the request queue.
void startTask(StringRef Name, unique_function<void()> Task,
Optional<WantDiagnostics> UpdateType);
+ /// Updates the TUStatus and emits it.
+ void emitTUStatus(TUAction FAction,
+ const TUStatus::BuildDetails *Detail = nullptr);
+
/// Determines the next action to perform.
/// All actions that should never run are discarded.
/// Returns a deadline for the next action. If it's expired, run now.
@@ -251,13 +255,17 @@
bool Done; /* GUARDED_BY(Mutex) */
std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
- /// Guards a critical section for running the diagnostics callbacks.
+ // FIXME: rename it to better fix the current usage, we also use it to guard
+ // emitting TUStatus.
+ /// Guards a critical section for running the diagnostics callbacks.
std::mutex DiagsMu;
// Used to prevent remove document + leading to out-of-order diagnostics:
// The lifetime of the old/new ASTWorkers will overlap, but their handles
// don't. When the old handle is destroyed, the old worker will stop reporting
// diagnostics.
bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */
+ /// Status of the TU.
+ TUStatus Status; /* GUARDED_BY(DiagMu) */
};
/// A smart-pointer-like class that points to an active ASTWorker.
@@ -340,6 +348,7 @@
}
void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
+ StringRef TaskName = "Update";
auto Task = [=]() mutable {
// Will be used to check if we can avoid rebuilding the AST.
bool InputsAreTheSame =
@@ -350,7 +359,7 @@
bool PrevDiagsWereReported = DiagsWereReported;
FileInputs = Inputs;
DiagsWereReported = false;
-
+ emitTUStatus({TUAction::BuildingPreamble, TaskName});
log("Updating file {0} with command [{1}] {2}", FileName,
Inputs.CompileCommand.Directory,
join(Inputs.CompileCommand.CommandLine, " "));
@@ -361,6 +370,9 @@
elog("Could not build CompilerInvocation for file {0}", FileName);
// Remove the old AST if it's still in cache.
IdleASTs.take(this);
+ TUStatus::BuildDetails Details;
+ Details.BuildFailed = true;
+ emitTUStatus({TUAction::BuildingPreamble, TaskName}, &Details);
// Make sure anyone waiting for the preamble gets notified it could not
// be built.
PreambleWasBuilt.notify();
@@ -386,7 +398,7 @@
// to it.
OldPreamble.reset();
PreambleWasBuilt.notify();
-
+ emitTUStatus({TUAction::BuildingFile, TaskName});
if (!CanReuseAST) {
IdleASTs.take(this); // Remove the old AST if it's still in cache.
} else {
@@ -403,6 +415,9 @@
// current file at this point?
log("Skipping rebuild of the AST for {0}, inputs are the same.",
FileName);
+ TUStatus::BuildDetails Details;
+ Details.ReuseAST = true;
+ emitTUStatus({TUAction::BuildingFile, TaskName}, &Details);
return;
}
}
@@ -443,8 +458,8 @@
// Stash the AST in the cache for further use.
IdleASTs.put(this, std::move(*AST));
};
-
- startTask("Update", std::move(Task), WantDiags);
+ emitTUStatus({TUAction::Queued, TaskName});
+ startTask(TaskName, std::move(Task), WantDiags);
}
void ASTWorker::runWithAST(
@@ -560,6 +575,18 @@
RequestsCV.notify_all();
}
+void ASTWorker::emitTUStatus(TUAction Action,
+ const TUStatus::BuildDetails *Details) {
+ std::lock_guard<std::mutex> Lock(DiagsMu);
+ // Stop emitting file statuses when the ASTWorker is shutting down.
+ if (ReportDiagnostics) {
+ Status.Action = std::move(Action);
+ if (Details)
+ Status.Details = *Details;
+ Callbacks.onFileUpdated(FileName, Status);
+ }
+}
+
void ASTWorker::run() {
while (true) {
Request Req;
@@ -598,12 +625,15 @@
std::lock_guard<Semaphore> BarrierLock(Barrier);
WithContext Guard(std::move(Req.Ctx));
trace::Span Tracer(Req.Name);
+ emitTUStatus({TUAction::RunningAction, Req.Name});
Req.Action();
}
{
std::lock_guard<std::mutex> Lock(Mutex);
Requests.pop_front();
+ if (Requests.empty())
+ emitTUStatus({TUAction::Idle, /*Name*/ ""});
}
RequestsCV.notify_all();
}
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -37,6 +37,7 @@
namespace clangd {
+// FIXME: find a better name.
class DiagnosticsConsumer {
public:
virtual ~DiagnosticsConsumer() = default;
@@ -44,6 +45,8 @@
/// Called by ClangdServer when \p Diagnostics for \p File are ready.
virtual void onDiagnosticsReady(PathRef File,
std::vector<Diag> Diagnostics) = 0;
+ /// Called whenever the file status is updated.
+ virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
};
/// Manages a collection of source files and derived data (ASTs, indexes),
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -86,6 +86,10 @@
DiagConsumer.onDiagnosticsReady(File, std::move(Diags));
}
+ void onFileUpdated(PathRef File, const TUStatus &Status) override {
+ DiagConsumer.onFileUpdated(File, Status);
+ }
+
private:
FileIndex *FIndex;
DiagnosticsConsumer &DiagConsumer;
@@ -144,6 +148,10 @@
void ClangdServer::addDocument(PathRef File, StringRef Contents,
WantDiagnostics WantDiags) {
+ // FIXME: some build systems like Bazel will take time to preparing
+ // environment to build the file, it would be nice if we could emit a
+ // "PreparingBuild" status to inform users, it is non-trivial given the
+ // current implementation.
WorkScheduler.update(File,
ParseInputs{getCompileCommand(File),
FSProvider.getFileSystem(), Contents.str()},
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
