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>> _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
