Re: [clang-tools-extra] r328500 - [clangd] Support incremental document syncing

2018-03-28 Thread Simon Marchi via cfe-commits 
On 2018-03-27 01:47 PM, Reid Kleckner wrote:
> One of these new tests does not pass on Windows due to assumptions about 
> absolute path structure. The FileCheck output seems self-explanatory:
> 
> $ "FileCheck" "-strict-whitespace" 
> "C:\b\slave\clang-x86-windows-msvc2015\clang-x86-windows-msvc2015\llvm\tools\clang\tools\extra\test\clangd\textdocument-didchange-fail.test"
> # command stderr:
> C:\b\slave\clang-x86-windows-msvc2015\clang-x86-windows-msvc2015\llvm\tools\clang\tools\extra\test\clangd\textdocument-didchange-fail.test:22:20:
>  error: expected string not found in input
> # CHECK-NEXT:      "uri": "file:///clangd-test/main.cpp"
>                    ^
> :68:7: note: scanning from here
>       "uri": "file:///C%3a/clangd-test/main.cpp"
> 
> I committed a speculative fix for this in r328645, hopefully it does the job.

Makes sense.  Thanks for catching and fixing it!  I received some failure
notifications, but at first sight they seemed unrelated (maybe some of them
were).  I'll look more closely next time.

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

Re: [clang-tools-extra] r328500 - [clangd] Support incremental document syncing

2018-03-27 Thread Reid Kleckner via cfe-commits 
One of these new tests does not pass on Windows due to assumptions about
absolute path structure. The FileCheck output seems self-explanatory:

$ "FileCheck" "-strict-whitespace"
"C:\b\slave\clang-x86-windows-msvc2015\clang-x86-windows-msvc2015\llvm\tools\clang\tools\extra\test\clangd\textdocument-didchange-fail.test"
# command stderr:
C:\b\slave\clang-x86-windows-msvc2015\clang-x86-windows-msvc2015\llvm\tools\clang\tools\extra\test\clangd\textdocument-didchange-fail.test:22:20:
error: expected string not found in input
# CHECK-NEXT:  "uri": "file:///clangd-test/main.cpp"
   ^
:68:7: note: scanning from here
  "uri": "file:///C%3a/clangd-test/main.cpp"

I committed a speculative fix for this in r328645, hopefully it does the
job.

On Mon, Mar 26, 2018 at 7:32 PM Simon Marchi via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: simark
> Date: Mon Mar 26 07:41:40 2018
> New Revision: 328500
>
> URL: http://llvm.org/viewvc/llvm-project?rev=328500=rev
> Log:
> [clangd] Support incremental document syncing
>
> Summary:
> This patch adds support for incremental document syncing, as described
> in the LSP spec.  The protocol specifies ranges in terms of Position (a
> line and a character), and our drafts are stored as plain strings.  So I
> see two things that may not be super efficient for very large files:
>
> - Converting a Position to an offset (the positionToOffset function)
>   requires searching for end of lines until we reach the desired line.
> - When we update a range, we construct a new string, which implies
>   copying the whole document.
>
> However, for the typical size of a C++ document and the frequency of
> update (at which a user types), it may not be an issue.  This patch aims
> at getting the basic feature in, and we can always improve it later if
> we find it's too slow.
>
> Signed-off-by: Simon Marchi 
>
> Reviewers: malaperle, ilya-biryukov
>
> Reviewed By: ilya-biryukov
>
> Subscribers: MaskRay, klimek, mgorny, ilya-biryukov, jkorous-apple,
> ioeric, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D44272
>
> Added:
> clang-tools-extra/trunk/test/clangd/textdocument-didchange-fail.test
> clang-tools-extra/trunk/unittests/clangd/DraftStoreTests.cpp
> Modified:
> clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
> clang-tools-extra/trunk/clangd/DraftStore.cpp
> clang-tools-extra/trunk/clangd/DraftStore.h
> clang-tools-extra/trunk/clangd/Protocol.cpp
> clang-tools-extra/trunk/clangd/Protocol.h
> clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
> clang-tools-extra/trunk/test/clangd/initialize-params.test
> clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
>
> Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=328500=328499=328500=diff
>
> ==
> --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
> +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Mon Mar 26 07:41:40
> 2018
> @@ -100,7 +100,7 @@ void ClangdLSPServer::onInitialize(Initi
>reply(json::obj{
>{{"capabilities",
>  json::obj{
> -{"textDocumentSync", 1},
> +{"textDocumentSync", (int)TextDocumentSyncKind::Incremental},
>  {"documentFormattingProvider", true},
>  {"documentRangeFormattingProvider", true},
>  {"documentOnTypeFormattingProvider",
> @@ -147,25 +147,30 @@ void ClangdLSPServer::onDocumentDidOpen(
>PathRef File = Params.textDocument.uri.file();
>std::string  = Params.textDocument.text;
>
> -  DraftMgr.updateDraft(File, Contents);
> +  DraftMgr.addDraft(File, Contents);
>Server.addDocument(File, Contents, WantDiagnostics::Yes);
>  }
>
>  void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams
> ) {
> -  if (Params.contentChanges.size() != 1)
> -return replyError(ErrorCode::InvalidParams,
> -  "can only apply one change at a time");
>auto WantDiags = WantDiagnostics::Auto;
>if (Params.wantDiagnostics.hasValue())
>  WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes
>: WantDiagnostics::No;
>
>PathRef File = Params.textDocument.uri.file();
> -  std::string  = Params.contentChanges[0].text;
> +  llvm::Expected Contents =
> +  DraftMgr.updateDraft(File, Params.contentChanges);
> +  if (!Contents) {
> +// If this fails, we are most likely going to be not in sync anymore
> with
> +// the client.  It is better to remove the draft and let further
> operations
> +// fail rather than giving wrong results.
> +DraftMgr.removeDraft(File);
> +Server.removeDocument(File);
> +log(llvm::toString(Contents.takeError()));
> +return;
> +  }
>
> -  // We only support

[clang-tools-extra] r328500 - [clangd] Support incremental document syncing

2018-03-26 Thread Simon Marchi via cfe-commits 
Author: simark
Date: Mon Mar 26 07:41:40 2018
New Revision: 328500

URL: http://llvm.org/viewvc/llvm-project?rev=328500=rev
Log:
[clangd] Support incremental document syncing

Summary:
This patch adds support for incremental document syncing, as described
in the LSP spec.  The protocol specifies ranges in terms of Position (a
line and a character), and our drafts are stored as plain strings.  So I
see two things that may not be super efficient for very large files:

- Converting a Position to an offset (the positionToOffset function)
  requires searching for end of lines until we reach the desired line.
- When we update a range, we construct a new string, which implies
  copying the whole document.

However, for the typical size of a C++ document and the frequency of
update (at which a user types), it may not be an issue.  This patch aims
at getting the basic feature in, and we can always improve it later if
we find it's too slow.

Signed-off-by: Simon Marchi 

Reviewers: malaperle, ilya-biryukov

Reviewed By: ilya-biryukov

Subscribers: MaskRay, klimek, mgorny, ilya-biryukov, jkorous-apple, ioeric, 
cfe-commits

Differential Revision: https://reviews.llvm.org/D44272

Added:
clang-tools-extra/trunk/test/clangd/textdocument-didchange-fail.test
clang-tools-extra/trunk/unittests/clangd/DraftStoreTests.cpp
Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/DraftStore.cpp
clang-tools-extra/trunk/clangd/DraftStore.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test
clang-tools-extra/trunk/test/clangd/initialize-params.test
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=328500=328499=328500=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Mon Mar 26 07:41:40 2018
@@ -100,7 +100,7 @@ void ClangdLSPServer::onInitialize(Initi
   reply(json::obj{
   {{"capabilities",
 json::obj{
-{"textDocumentSync", 1},
+{"textDocumentSync", (int)TextDocumentSyncKind::Incremental},
 {"documentFormattingProvider", true},
 {"documentRangeFormattingProvider", true},
 {"documentOnTypeFormattingProvider",
@@ -147,25 +147,30 @@ void ClangdLSPServer::onDocumentDidOpen(
   PathRef File = Params.textDocument.uri.file();
   std::string  = Params.textDocument.text;
 
-  DraftMgr.updateDraft(File, Contents);
+  DraftMgr.addDraft(File, Contents);
   Server.addDocument(File, Contents, WantDiagnostics::Yes);
 }
 
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams ) 
{
-  if (Params.contentChanges.size() != 1)
-return replyError(ErrorCode::InvalidParams,
-  "can only apply one change at a time");
   auto WantDiags = WantDiagnostics::Auto;
   if (Params.wantDiagnostics.hasValue())
 WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes
   : WantDiagnostics::No;
 
   PathRef File = Params.textDocument.uri.file();
-  std::string  = Params.contentChanges[0].text;
+  llvm::Expected Contents =
+  DraftMgr.updateDraft(File, Params.contentChanges);
+  if (!Contents) {
+// If this fails, we are most likely going to be not in sync anymore with
+// the client.  It is better to remove the draft and let further operations
+// fail rather than giving wrong results.
+DraftMgr.removeDraft(File);
+Server.removeDocument(File);
+log(llvm::toString(Contents.takeError()));
+return;
+  }
 
-  // We only support full syncing right now.
-  DraftMgr.updateDraft(File, Contents);
-  Server.addDocument(File, Contents, WantDiags);
+  Server.addDocument(File, *Contents, WantDiags);
 }
 
 void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams ) {

Modified: clang-tools-extra/trunk/clangd/DraftStore.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/DraftStore.cpp?rev=328500=328499=328500=diff
==
--- clang-tools-extra/trunk/clangd/DraftStore.cpp (original)
+++ clang-tools-extra/trunk/clangd/DraftStore.cpp Mon Mar 26 07:41:40 2018
@@ -8,6 +8,8 @@
 
//===--===//
 
 #include "DraftStore.h"
+#include "SourceCode.h"
+#include "llvm/Support/Errc.h"
 
 using namespace clang;
 using namespace clang::clangd;
@@ -32,12 +34,72 @@ std::vector DraftStore::getActiveF
   return ResultVector;
 }
 
-void DraftStore::updateDraft(PathRef File, StringRef Contents) {
+void