[PATCH] D82642: [clangd] Run formatting operations asynchronously.

2020-06-30 Thread Sam McCall via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
sammccall marked 2 inline comments as done.
Closed by commit rGffa63dde8e97: [clangd] Run formatting operations 
asynchronously. (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82642/new/

https://reviews.llvm.org/D82642

Files:
  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

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -44,6 +44,9 @@
 Position Pos, StringRef NewName,
 const clangd::RenameOptions );
 
+llvm::Expected
+runFormatFile(ClangdServer , PathRef File, StringRef Code);
+
 std::string runDumpAST(ClangdServer , PathRef File);
 
 llvm::Expected>
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -105,6 +105,13 @@
   return std::move(*Result);
 }
 
+llvm::Expected
+runFormatFile(ClangdServer , PathRef File, StringRef Code) {
+  llvm::Optional> Result;
+  Server.formatFile(File, Code, capture(Result));
+  return std::move(*Result);
+}
+
 std::string runDumpAST(ClangdServer , PathRef File) {
   llvm::Optional Result;
   Server.dumpAST(File, capture(Result));
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -878,7 +878,7 @@
   FS.Files[Path] = Code;
   runAddDocument(Server, Path, Code);
 
-  auto Replaces = Server.formatFile(Code, Path);
+  auto Replaces = runFormatFile(Server, Path, Code);
   EXPECT_TRUE(static_cast(Replaces));
   auto Changed = tooling::applyAllReplacements(Code, *Replaces);
   EXPECT_TRUE(static_cast(Changed));
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -247,18 +247,17 @@
   Callback CB);
 
   /// Run formatting for \p Rng inside \p File with content \p Code.
-  llvm::Expected formatRange(StringRef Code,
-PathRef File, Range Rng);
+  void formatRange(PathRef File, StringRef Code, Range Rng,
+   Callback CB);
 
   /// Run formatting for the whole \p File with content \p Code.
-  llvm::Expected formatFile(StringRef Code,
-   PathRef File);
+  void formatFile(PathRef File, StringRef Code,
+  Callback CB);
 
   /// Run formatting after \p TriggerText was typed at \p Pos in \p File with
   /// content \p Code.
-  llvm::Expected> formatOnType(StringRef Code,
- PathRef File, Position Pos,
- StringRef TriggerText);
+  void formatOnType(PathRef File, StringRef Code, Position Pos,
+StringRef TriggerText, Callback> CB);
 
   /// Test the validity of a rename operation.
   void prepareRename(PathRef File, Position Pos,
@@ -323,11 +322,9 @@
   blockUntilIdleForTest(llvm::Optional 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
-  formatCode(llvm::StringRef Code, PathRef File,
- ArrayRef Ranges);
+  void formatCode(PathRef File, llvm::StringRef Code,
+  ArrayRef Ranges,
+  Callback CB);
 
   const ThreadsafeFS 
 
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -296,40 +296,46 @@
 std::move(Action));
 }
 
-llvm::Expected
-ClangdServer::formatRange(llvm::StringRef Code, PathRef File, Range Rng) {
+void ClangdServer::formatRange(PathRef File, llvm::StringRef Code, Range Rng,
+   Callback CB) {
   llvm::Expected Begin = positionToOffset(Code, Rng.start);
   if (!Begin)
-return Begin.takeError();
+return CB(Begin.takeError());
   llvm::Expected End = positionToOffset(Code, Rng.end);
   if (!End)
-return End.takeError();
-  return formatCode(Code, File, {tooling::Range(*Begin, *End - *Begin)});
+

[PATCH] D82642: [clangd] Run formatting operations asynchronously.

2020-06-30 Thread Sam McCall via Phabricator via cfe-commits
sammccall marked 4 inline comments as done.
sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:900
+  llvm::Expected Result) mutable {
+if (Result)
+  Reply(replacementsToEdits(Code, Result.get()));

kbobyrev wrote:
> `Reply(Result ? replacementsToEdits(...) : Result.takeError())` may be 
> shorter, but up to you, maybe this looks more readable.
Sadly it doesn't compile... the usual thing about not being able to determine a 
common type for the two expressions, and C++ doesn't infer it from the type 
that's required in the context.



Comment at: clang-tools-extra/clangd/ClangdServer.h:326
 private:
-  /// FIXME: This stats several files to find a .clang-format file. I/O can be
-  /// slow. Think of a way to cache this.

kbobyrev wrote:
> Is this no longer true?
It's still true that we do IO and it might be nice to cache it.
However this same IO (to get the formatstyle) also now happens in lots of other 
places too (without comments), and the only reason this one was especially bad 
is that it happened on the main thread (which is no longer true).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82642/new/

https://reviews.llvm.org/D82642



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D82642: [clangd] Run formatting operations asynchronously.

2020-06-30 Thread Kirill Bobyrev via Phabricator via cfe-commits
kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.

Looks nice just a couple of nits.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:900
+  llvm::Expected Result) mutable {
+if (Result)
+  Reply(replacementsToEdits(Code, Result.get()));

`Reply(Result ? replacementsToEdits(...) : Result.takeError())` may be shorter, 
but up to you, maybe this looks more readable.



Comment at: clang-tools-extra/clangd/ClangdServer.h:326
 private:
-  /// FIXME: This stats several files to find a .clang-format file. I/O can be
-  /// slow. Think of a way to cache this.

Is this no longer true?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82642/new/

https://reviews.llvm.org/D82642



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D82642: [clangd] Run formatting operations asynchronously.

2020-06-26 Thread Sam McCall via Phabricator via cfe-commits
sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82642

Files:
  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

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -44,6 +44,9 @@
 Position Pos, StringRef NewName,
 const clangd::RenameOptions );
 
+llvm::Expected
+runFormatFile(ClangdServer , PathRef File, StringRef Code);
+
 std::string runDumpAST(ClangdServer , PathRef File);
 
 llvm::Expected>
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -105,6 +105,13 @@
   return std::move(*Result);
 }
 
+llvm::Expected
+runFormatFile(ClangdServer , PathRef File, StringRef Code) {
+  llvm::Optional> Result;
+  Server.formatFile(File, Code, capture(Result));
+  return std::move(*Result);
+}
+
 std::string runDumpAST(ClangdServer , PathRef File) {
   llvm::Optional Result;
   Server.dumpAST(File, capture(Result));
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -877,7 +877,7 @@
   FS.Files[Path] = Code;
   runAddDocument(Server, Path, Code);
 
-  auto Replaces = Server.formatFile(Code, Path);
+  auto Replaces = runFormatFile(Server, Path, Code);
   EXPECT_TRUE(static_cast(Replaces));
   auto Changed = tooling::applyAllReplacements(Code, *Replaces);
   EXPECT_TRUE(static_cast(Changed));
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -247,18 +247,17 @@
   Callback CB);
 
   /// Run formatting for \p Rng inside \p File with content \p Code.
-  llvm::Expected formatRange(StringRef Code,
-PathRef File, Range Rng);
+  void formatRange(PathRef File, StringRef Code, Range Rng,
+   Callback CB);
 
   /// Run formatting for the whole \p File with content \p Code.
-  llvm::Expected formatFile(StringRef Code,
-   PathRef File);
+  void formatFile(PathRef File, StringRef Code,
+  Callback CB);
 
   /// Run formatting after \p TriggerText was typed at \p Pos in \p File with
   /// content \p Code.
-  llvm::Expected> formatOnType(StringRef Code,
- PathRef File, Position Pos,
- StringRef TriggerText);
+  void formatOnType(PathRef File, StringRef Code, Position Pos,
+StringRef TriggerText, Callback> CB);
 
   /// Test the validity of a rename operation.
   void prepareRename(PathRef File, Position Pos,
@@ -323,11 +322,9 @@
   blockUntilIdleForTest(llvm::Optional 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
-  formatCode(llvm::StringRef Code, PathRef File,
- ArrayRef Ranges);
+  void formatCode(PathRef File, llvm::StringRef Code,
+  ArrayRef Ranges,
+  Callback CB);
 
   const ThreadsafeFS 
 
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -296,40 +296,46 @@
 std::move(Action));
 }