This revision was automatically updated to reflect the committed changes.
Closed by commit rL328500: [clangd] Support incremental document syncing
(authored by simark, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM.
Comment at: unittests/clangd/DraftStoreTests.cpp:1
+//===-- DraftStoreTests.cpp - Clangd unit tests -*- C++
-*-===//
+//
simark updated this revision to Diff 139596.
simark marked 2 inline comments as done.
simark added a comment.
Address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
Files:
clangd/ClangdLSPServer.cpp
clangd/DraftStore.cpp
clangd/DraftStore.h
simark marked 10 inline comments as done.
simark added inline comments.
Comment at: clangd/DraftStore.cpp:75
+ return llvm::make_error(
+ llvm::formatv("Range's end position (line={0}, character={1}) is "
+"before start position (line={2},
ilya-biryukov added a comment.
Thanks again. This generally looks LGTM, just a last drop of small nits. Will
approve as soon as they land.
Comment at: clangd/DraftStore.cpp:75
+ return llvm::make_error(
+ llvm::formatv("Range's end position (line={0},
simark added inline comments.
Comment at: clangd/DraftStore.h:36
/// Replace contents of the draft for \p File with \p Contents.
- void updateDraft(PathRef File, StringRef Contents);
+ void addDraft(PathRef File, StringRef Contents);
+
ilya-biryukov wrote:
ilya-biryukov added a comment.
Thanks for addressing the comments. I have just one comment left, about the LSP
versions and sanity-checking.
Comment at: clangd/DraftStore.h:36
/// Replace contents of the draft for \p File with \p Contents.
- void updateDraft(PathRef File,
simark updated this revision to Diff 139355.
simark marked an inline comment as done.
simark added a comment.
Address review comments I failed to address previously
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
Files:
clangd/ClangdLSPServer.cpp
clangd/DraftStore.cpp
simark marked 6 inline comments as done.
simark added inline comments.
Herald added a subscriber: MaskRay.
Comment at: unittests/clangd/DraftStoreTests.cpp:27
+
+static int rangeLength(StringRef Code, const Range ) {
+ size_t Start = positionToOffset(Code, Rng.start);
ilya-biryukov added inline comments.
Comment at: unittests/clangd/DraftStoreTests.cpp:27
+
+static int rangeLength(StringRef Code, const Range ) {
+ size_t Start = positionToOffset(Code, Rng.start);
ilya-biryukov wrote:
> no need for `static`, since function is
simark updated this revision to Diff 139328.
simark added a comment.
Add lit test case
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
Files:
clangd/ClangdLSPServer.cpp
clangd/DraftStore.cpp
clangd/DraftStore.h
clangd/Protocol.cpp
clangd/Protocol.h
simark updated this revision to Diff 139321.
simark added a comment.
Remove draft if update fails
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
Files:
clangd/ClangdLSPServer.cpp
clangd/DraftStore.cpp
clangd/DraftStore.h
clangd/Protocol.cpp
clangd/Protocol.h
ilya-biryukov added inline comments.
Comment at: clangd/DraftStore.h:36
/// Replace contents of the draft for \p File with \p Contents.
- void updateDraft(PathRef File, StringRef Contents);
+ void addDraft(PathRef File, StringRef Contents);
+
simark wrote:
simark updated this revision to Diff 139310.
simark marked an inline comment as done.
simark added a comment.
Address review comments, except LSP version check
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
Files:
clangd/ClangdLSPServer.cpp
clangd/DraftStore.cpp
simark marked 34 inline comments as done.
simark added inline comments.
Comment at: clangd/DraftStore.h:36
/// Replace contents of the draft for \p File with \p Contents.
- void updateDraft(PathRef File, StringRef Contents);
+ void addDraft(PathRef File, StringRef
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:164
+ if (!Contents) {
+log(llvm::toString(Contents.takeError()));
+return;
simark wrote:
> ilya-biryukov wrote:
> > We should signal an error to the client by calling
simark marked 3 inline comments as done.
simark added inline comments.
Comment at: clangd/DraftStore.cpp:61
+ const Position = Change.range->start;
+ size_t StartIndex = positionToOffset(Contents, Start);
+ if (StartIndex > Contents.length()) {
simark marked 4 inline comments as done.
simark added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:164
+ if (!Contents) {
+log(llvm::toString(Contents.takeError()));
+return;
ilya-biryukov wrote:
> We should signal an error to the client by
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:164
+ if (!Contents) {
+log(llvm::toString(Contents.takeError()));
+return;
We should signal an error to the client by calling `replyError`
Comment at:
simark updated this revision to Diff 138741.
simark marked an inline comment as done.
simark added a comment.
Address review comments, rebase on latest master
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
Files:
clangd/ClangdLSPServer.cpp
clangd/DraftStore.cpp
simark marked 3 inline comments as done.
simark added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:101
json::obj{
-{"textDocumentSync", 1},
+{"textDocumentSync", 2},
{"documentFormattingProvider", true},
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:101
json::obj{
-{"textDocumentSync", 1},
+{"textDocumentSync", 2},
{"documentFormattingProvider", true},
simark wrote:
> ilya-biryukov
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:149
void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams )
{
if (Params.contentChanges.size() != 1)
return replyError(ErrorCode::InvalidParams,
simark wrote:
>
simark added a comment.
I uploaded a new patch that moves the draft store to ClangdLSPServer, that
implements what you suggested.
https://reviews.llvm.org/D44408
I will update the incremental sync patch once that patch is in.
Repository:
rCTE Clang Tools Extra
simark marked 2 inline comments as done.
simark added inline comments.
Comment at: clangd/ClangdServer.h:159
/// constructor will receive onDiagnosticsReady callback.
void addDocument(PathRef File, StringRef Contents,
+ WantDiagnostics WantDiags =
simark marked 2 inline comments as done.
simark added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:101
json::obj{
-{"textDocumentSync", 1},
+{"textDocumentSync", 2},
{"documentFormattingProvider", true},
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:557
// 64-bit unsigned integer.
if (Version < LastReportedDiagsVersion)
return;
When you'll try remove the `DraftMgr`, this piece of code will be hard to
refactor because
ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/ClangdLSPServer.cpp:101
json::obj{
-{"textDocumentSync", 1},
+{"textDocumentSync", 2},
simark created this revision.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov,
mgorny, klimek.
simark added a reviewer: malaperle.
This patch adds support for incremental document syncing, as described
in the LSP spec. The protocol specifies ranges in terms of
29 matches
Mail list logo
