[clang-tools-extra] ffa63dd - [clangd] Run formatting operations asynchronously.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 30 15:52:18 PDT 2020
Author: Sam McCall
Date: 2020-07-01T00:52:08+02:00
New Revision: ffa63dde8e97a34b8914a151556551f74d4227e7
URL: https://github.com/llvm/llvm-project/commit/ffa63dde8e97a34b8914a151556551f74d4227e7
DIFF: https://github.com/llvm/llvm-project/commit/ffa63dde8e97a34b8914a151556551f74d4227e7.diff
LOG: [clangd] Run formatting operations asynchronously.
Summary:
They don't need ASTs or anything, so they should still run immediately.
These were sync for historical reasons (they predate clangd having a pervasive
threading model). This worked ok as they were "cheap".
Aside for consistency, there are a couple of reasons to make them async:
- they do IO (finding .clang-format) so aren't trivially cheap
- having TUScheduler involved in running these tasks means we can use it as
an injection point for configuration.
(TUScheduler::run will need to learn about which file is being operated on,
but that's an easy change).
- adding the config system adds more potential IO, too
Reviewers: kbobyrev
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D82642
Added:
Modified:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/unittests/ClangdTests.cpp
clang-tools-extra/clangd/unittests/SyncAPI.cpp
clang-tools-extra/clangd/unittests/SyncAPI.h
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 93f2746bb6f0..1d794b1898c7 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -879,7 +879,8 @@ void ClangdLSPServer::onDocumentOnTypeFormatting(
"onDocumentOnTypeFormatting called for non-added file",
ErrorCode::InvalidParams));
- Reply(Server->formatOnType(Code->Contents, File, Params.position, Params.ch));
+ Server->formatOnType(File, Code->Contents, Params.position, Params.ch,
+ std::move(Reply));
}
void ClangdLSPServer::onDocumentRangeFormatting(
@@ -892,12 +893,15 @@ void ClangdLSPServer::onDocumentRangeFormatting(
"onDocumentRangeFormatting called for non-added file",
ErrorCode::InvalidParams));
- auto ReplacementsOrError =
- Server->formatRange(Code->Contents, File, Params.range);
- if (ReplacementsOrError)
- Reply(replacementsToEdits(Code->Contents, ReplacementsOrError.get()));
- else
- Reply(ReplacementsOrError.takeError());
+ Server->formatRange(
+ File, Code->Contents, Params.range,
+ [Code = Code->Contents, Reply = std::move(Reply)](
+ llvm::Expected<tooling::Replacements> Result) mutable {
+ if (Result)
+ Reply(replacementsToEdits(Code, Result.get()));
+ else
+ Reply(Result.takeError());
+ });
}
void ClangdLSPServer::onDocumentFormatting(
@@ -910,11 +914,14 @@ void ClangdLSPServer::onDocumentFormatting(
"onDocumentFormatting called for non-added file",
ErrorCode::InvalidParams));
- auto ReplacementsOrError = Server->formatFile(Code->Contents, File);
- if (ReplacementsOrError)
- Reply(replacementsToEdits(Code->Contents, ReplacementsOrError.get()));
- else
- Reply(ReplacementsOrError.takeError());
+ Server->formatFile(File, Code->Contents,
+ [Code = Code->Contents, Reply = std::move(Reply)](
+ llvm::Expected<tooling::Replacements> Result) mutable {
+ if (Result)
+ Reply(replacementsToEdits(Code, Result.get()));
+ else
+ Reply(Result.takeError());
+ });
}
/// The functions constructs a flattened view of the DocumentSymbol hierarchy.
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 6c7255c96963..b33d53699405 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -296,40 +296,46 @@ void ClangdServer::signatureHelp(PathRef File, Position Pos,
std::move(Action));
}
-llvm::Expected<tooling::Replacements>
-ClangdServer::formatRange(llvm::StringRef Code, PathRef File, Range Rng) {
+void ClangdServer::formatRange(PathRef File, llvm::StringRef Code, Range Rng,
+ Callback<tooling::Replacements> CB) {
llvm::Expected<size_t> Begin = positionToOffset(Code, Rng.start);
if (!Begin)
- return Begin.takeError();
+ return CB(Begin.takeError());
llvm::Expected<size_t> End = positionToOffset(Code, Rng.end);
if (!End)
- return End.takeError();
- return formatCode(Code, File, {tooling::Range(*Begin, *End - *Begin)});
+ return CB(End.takeError());
+ formatCode(File, Code, {tooling::Range(*Begin, *End - *Begin)},
+ std::move(CB));
}
-llvm::Expected<tooling::Replacements>
-ClangdServer::formatFile(llvm::StringRef Code, PathRef File) {
+void ClangdServer::formatFile(PathRef File, llvm::StringRef Code,
+ Callback<tooling::Replacements> CB) {
// Format everything.
- return formatCode(Code, File, {tooling::Range(0, Code.size())});
+ formatCode(File, Code, {tooling::Range(0, Code.size())}, std::move(CB));
}
-llvm::Expected<std::vector<TextEdit>>
-ClangdServer::formatOnType(llvm::StringRef Code, PathRef File, Position Pos,
- StringRef TriggerText) {
+void ClangdServer::formatOnType(PathRef File, llvm::StringRef Code,
+ Position Pos, StringRef TriggerText,
+ Callback<std::vector<TextEdit>> CB) {
llvm::Expected<size_t> CursorPos = positionToOffset(Code, Pos);
if (!CursorPos)
- return CursorPos.takeError();
- auto Style = format::getStyle(format::DefaultFormatStyle, File,
- format::DefaultFallbackStyle, Code,
- TFS.view(/*CWD=*/llvm::None).get());
- if (!Style)
- return Style.takeError();
-
- std::vector<TextEdit> Result;
- for (const tooling::Replacement &R :
- formatIncremental(Code, *CursorPos, TriggerText, *Style))
- Result.push_back(replacementToEdit(Code, R));
- return Result;
+ return CB(CursorPos.takeError());
+ auto Action = [File = File.str(), Code = Code.str(),
+ TriggerText = TriggerText.str(), CursorPos = *CursorPos,
+ CB = std::move(CB), this]() mutable {
+ auto Style = format::getStyle(format::DefaultFormatStyle, File,
+ format::DefaultFallbackStyle, Code,
+ TFS.view(/*CWD=*/llvm::None).get());
+ if (!Style)
+ return CB(Style.takeError());
+
+ std::vector<TextEdit> Result;
+ for (const tooling::Replacement &R :
+ formatIncremental(Code, CursorPos, TriggerText, *Style))
+ Result.push_back(replacementToEdit(Code, R));
+ return CB(Result);
+ };
+ WorkScheduler.run("FormatOnType", std::move(Action));
}
void ClangdServer::prepareRename(PathRef File, Position Pos,
@@ -561,21 +567,25 @@ void ClangdServer::switchSourceHeader(
WorkScheduler.runWithAST("SwitchHeaderSource", Path, std::move(Action));
}
-llvm::Expected<tooling::Replacements>
-ClangdServer::formatCode(llvm::StringRef Code, PathRef File,
- llvm::ArrayRef<tooling::Range> Ranges) {
+void ClangdServer::formatCode(PathRef File, llvm::StringRef Code,
+ llvm::ArrayRef<tooling::Range> Ranges,
+ Callback<tooling::Replacements> CB) {
// Call clang-format.
- format::FormatStyle Style = getFormatStyleForFile(File, Code, TFS);
- tooling::Replacements IncludeReplaces =
- format::sortIncludes(Style, Code, Ranges, File);
- auto Changed = tooling::applyAllReplacements(Code, IncludeReplaces);
- if (!Changed)
- return Changed.takeError();
-
- return IncludeReplaces.merge(format::reformat(
- Style, *Changed,
- tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
- File));
+ auto Action = [File = File.str(), Code = Code.str(), Ranges = Ranges.vec(),
+ CB = std::move(CB), this]() mutable {
+ format::FormatStyle Style = getFormatStyleForFile(File, Code, TFS);
+ tooling::Replacements IncludeReplaces =
+ format::sortIncludes(Style, Code, Ranges, File);
+ auto Changed = tooling::applyAllReplacements(Code, IncludeReplaces);
+ if (!Changed)
+ return CB(Changed.takeError());
+
+ CB(IncludeReplaces.merge(format::reformat(
+ Style, *Changed,
+ tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
+ File)));
+ };
+ WorkScheduler.run("Format", std::move(Action));
}
void ClangdServer::findDocumentHighlights(
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 021540df66f3..c2a5c98fc458 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -247,18 +247,17 @@ class ClangdServer {
Callback<ReferencesResult> CB);
/// Run formatting for \p Rng inside \p File with content \p Code.
- llvm::Expected<tooling::Replacements> formatRange(StringRef Code,
- PathRef File, Range Rng);
+ void formatRange(PathRef File, StringRef Code, Range Rng,
+ Callback<tooling::Replacements> CB);
/// Run formatting for the whole \p File with content \p Code.
- llvm::Expected<tooling::Replacements> formatFile(StringRef Code,
- PathRef File);
+ void formatFile(PathRef File, StringRef Code,
+ Callback<tooling::Replacements> CB);
/// Run formatting after \p TriggerText was typed at \p Pos in \p File with
/// content \p Code.
- llvm::Expected<std::vector<TextEdit>> formatOnType(StringRef Code,
- PathRef File, Position Pos,
- StringRef TriggerText);
+ void formatOnType(PathRef File, StringRef Code, Position Pos,
+ StringRef TriggerText, Callback<std::vector<TextEdit>> CB);
/// Test the validity of a rename operation.
void prepareRename(PathRef File, Position Pos,
@@ -323,11 +322,9 @@ class ClangdServer {
blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);
private:
- /// FIXME: This stats several files to find a .clang-format file. I/O can be
- /// slow. Think of a way to cache this.
- llvm::Expected<tooling::Replacements>
- formatCode(llvm::StringRef Code, PathRef File,
- ArrayRef<tooling::Range> Ranges);
+ void formatCode(PathRef File, llvm::StringRef Code,
+ ArrayRef<tooling::Range> Ranges,
+ Callback<tooling::Replacements> CB);
const ThreadsafeFS &TFS;
diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
index 04e97d0fa3f9..943a3c7925f0 100644
--- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -878,7 +878,7 @@ void f() {}
FS.Files[Path] = Code;
runAddDocument(Server, Path, Code);
- auto Replaces = Server.formatFile(Code, Path);
+ auto Replaces = runFormatFile(Server, Path, Code);
EXPECT_TRUE(static_cast<bool>(Replaces));
auto Changed = tooling::applyAllReplacements(Code, *Replaces);
EXPECT_TRUE(static_cast<bool>(Changed));
diff --git a/clang-tools-extra/clangd/unittests/SyncAPI.cpp b/clang-tools-extra/clangd/unittests/SyncAPI.cpp
index e976b5ab9389..222d6683f49b 100644
--- a/clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ b/clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -105,6 +105,13 @@ llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File,
return std::move(*Result);
}
+llvm::Expected<tooling::Replacements>
+runFormatFile(ClangdServer &Server, PathRef File, StringRef Code) {
+ llvm::Optional<llvm::Expected<tooling::Replacements>> Result;
+ Server.formatFile(File, Code, capture(Result));
+ return std::move(*Result);
+}
+
std::string runDumpAST(ClangdServer &Server, PathRef File) {
llvm::Optional<std::string> Result;
Server.dumpAST(File, capture(Result));
diff --git a/clang-tools-extra/clangd/unittests/SyncAPI.h b/clang-tools-extra/clangd/unittests/SyncAPI.h
index 22b03c6f1fa5..617c1e5a1954 100644
--- a/clang-tools-extra/clangd/unittests/SyncAPI.h
+++ b/clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -44,6 +44,9 @@ llvm::Expected<FileEdits> runRename(ClangdServer &Server, PathRef File,
Position Pos, StringRef NewName,
const clangd::RenameOptions &RenameOpts);
+llvm::Expected<tooling::Replacements>
+runFormatFile(ClangdServer &Server, PathRef File, StringRef Code);
+
std::string runDumpAST(ClangdServer &Server, PathRef File);
llvm::Expected<std::vector<SymbolInformation>>
More information about the cfe-commits
mailing list