[PATCH] D44272: [clangd] Support incremental document syncing
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 19 08:33:47 PDT 2018
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: clangd/ClangdLSPServer.cpp:149
void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
if (Params.contentChanges.size() != 1)
return replyError(ErrorCode::InvalidParams,
----------------
simark wrote:
> ilya-biryukov wrote:
> > simark wrote:
> > > ilya-biryukov wrote:
> > > > We should handle more than a single change here.
> > > Ok, I'll try that. I am not sure I interpret the spec correctly. If you have two changes, the second change applies to the state of the document after having applied the first change, is that right? If that's the case, I think we only need to iterate on the changes and call addDocument on them sequentially (which could be done in the DraftMgr, given the interface change to DraftStore you suggest lower).
> > I am interpreting it in the same manner, i.e. we need to apply the edits in the sequence they were provided.
> > But I would double-check on a client that actually send multiple changes. Does VSCode do that?
> I don't know of any client that does that. AFAIK, vscode doesn't:
>
> https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/client.ts#L907-L908
>
> It's the only place I could find where a `DidChangeTextDocumentParams` is created, and it comes from a single `TextDocumentChangeEvent`, so there will always be only one item in the array.
But `TextDocumentChangeEvent` may contain multiple content changes, see [[ https://github.com/Microsoft/vscode-languageserver-node/blob/6501c8f2b79afcd8f18afd3a247c4566c2f67471/client/src/codeConverter.ts#L141 | codeConverter.ts ]].
Anyway, the spec seems to be clear enough here. Let's support multiple changes, it should be super-easy on our side (just call a function that does the update in a loop)
================
Comment at: clangd/DraftStore.cpp:47
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+ llvm::Optional<Range> range,
+ std::string *NewContents) {
----------------
simark wrote:
> ilya-biryukov wrote:
> > simark wrote:
> > > ilya-biryukov wrote:
> > > > NIT: LLVM uses `UpperCamelCase` for parameters and local variables
> > > Woops. I should learn to use clang-tidy. It found other instances (the local variables) but it doesn't find the parameters not written in camel case. Do you have an idea why? I dumped the config and see these:
> > >
> > > ```
> > > - key: readability-identifier-naming.ClassCase
> > > value: CamelCase
> > > - key: readability-identifier-naming.EnumCase
> > > value: CamelCase
> > > - key: readability-identifier-naming.UnionCase
> > > value: CamelCase
> > > - key: readability-identifier-naming.VariableCase
> > > value: CamelCase
> > > ```
> > >
> > > I assume there must be a `ParamCase` or something like that, but I can't find the exhaustive list of parameters for that check.
> > There is indeed a `readability-idenfitier.ParameterCase` key.
> > Found in the code, it is probably missing from the docs. [[ https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/readability/IdentifierNamingCheck.cpp#L67 | IdentifierNamingCheck.cpp ]].
> > We should probably update the relevant clang-tidy config. (not sure where it is, though, I don't use clang-tidy for clangd development. should really start doing that :-))
> The .clang-tidy file in the root of the llvm repo contains it. But the one in the clang repo overrides the `readability-identifier-naming` section without defining `ParameterCase`. That's probably why it's not enabled.
>
> ```
> 'llvm-header-guard' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'llvm-include-order' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'llvm-namespace-comment' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'llvm-twine-local' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-definitions-in-headers' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-misplaced-const' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-new-delete-overloads' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-non-copyable-objects' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-redundant-expression' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-static-assert' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-throw-by-value-catch-by-reference' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-unconventional-assign-operator' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-uniqueptr-reset-release' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-unused-alias-decls' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'misc-unused-using-decls' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> 'readability-identifier-naming' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
> ```
>
> So we should probably update the .clang-tidy file in the clang repo?
Yeah, it looks outdated, .clang_tidy in top-level llvm directory already has `ParameterCase`. Created D44628 to update the config in clang too.
================
Comment at: clangd/DraftStore.cpp:61
+ const Position &Start = Change.range->start;
+ size_t StartIndex = positionToOffset(Contents, Start);
+ if (StartIndex > Contents.length()) {
----------------
`positionToOffset` always truncates to size of contents. It has to return `Expected<int>` too here.
================
Comment at: clangd/DraftStore.cpp:64
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("Range's start position (line={0}, character={1}) is "
+ "out of range.",
----------------
NIT: maybe use `change` or `edit` instead of `range`?
`range's start position .... is out of range` is a bit confusing.
================
Comment at: clangd/DraftStore.cpp:73
+ if (EndIndex > Contents.length()) {
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("Range's end position (line={0}, character={1}) is "
----------------
Having partially applied changes in the map seems weird. Maybe the code applying the diffs to a separate function that returns either new contents or an error and use it here?
I.e. we won't be able to end up in a state with partially applied changes:
```
Expected<string> applyChanges(StringRef Contents, ArrayRef<TextDocumentChange> Changes) {
}
// See other comment about Version.
Expected<string> updateDraft(StringRef File, /*int Version,*/ ArrayRef<TextDocumentChange> Changes) {
// check File is in the map and version matches....
//...
string& Contents = ...;
auto NewContents = applyChanges(Contents, Changes);
if (!NewContents)
return NewContents.takeError();
Contents = *NewContents;
return std::move(*NewContents);
}
```
================
Comment at: clangd/DraftStore.cpp:97
+
+ NewContents.reserve(StartIndex + Change.text.length() +
+ (Contents.length() - EndIndex));
----------------
The math is correct, but I'm confused on how to properly read the formula here.
Maybe change to:
`Contents.length() - (EndIndex - StartIndex) + Change.text.length()`?
This clearly reads as:
`LengthBefore - LengthRemoved + LengthAdded`
================
Comment at: clangd/DraftStore.cpp:103
+
+ if (EndIndex < Contents.length())
+ NewContents += Contents.substr(EndIndex);
----------------
This check seems unnecessary, we've already checked for range errors. Maybe remove it?
================
Comment at: clangd/DraftStore.cpp:106
+ } else {
+ NewContents = Change.text;
+ }
----------------
It is impossible to mix full content changes with incremental range changes, right?
I suggest handling the full content change as a separate case at the start of the function:
```
if (Changes.size() == 1 && !Changes[0].range) {
Contents = std::move(Change.text);
return Contents;
}
for (auto &Change : Changes) {
if (!Change.range)
return make_error("Full change in the middle of incremental changes");
}
```
================
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);
+
----------------
Could we add versions from LSP's `VersionedTextDocumentIdentifier` here and make the relevant sanity checks?
Applying diffs to the wrong version will cause everything to fall apart, so we should detect this error and signal it to the client as soon as possible.
================
Comment at: unittests/clangd/DraftStoreTests.cpp:27
+
+static int rangeLength(StringRef Code, const Range &Rng) {
+ size_t Start = positionToOffset(Code, Rng.start);
----------------
no need for `static`, since function is already inside an anonymous namespace.
remove it?
================
Comment at: unittests/clangd/DraftStoreTests.cpp:38
+// Send the changes one by one to updateDraft, verify the intermediate results.
+
+static void stepByStep(llvm::ArrayRef<IncrementalTestStep> Steps) {
----------------
NIT: remove empty line after comment?
NIT: make the comment doxygen-like (i.e. start with `///`)?
================
Comment at: unittests/clangd/DraftStoreTests.cpp:42
+ Annotations InitialSrc(Steps.front().Src);
+ const char Path[] = "/hello.cpp";
+
----------------
NIT: use `constexpr StringLiteral Path("/hello.cpp")` instead?
It's a `StringRef` with size known at compile-time.
================
Comment at: unittests/clangd/DraftStoreTests.cpp:58
+ llvm::Expected<std::string> Result = DS.updateDraft(Path, {Event});
+ EXPECT_TRUE(!!Result);
+ EXPECT_EQ(*Result, SrcAfter.code());
----------------
Use `ASSERT_TRUE(!!Result)`, the code below won't work for failure results anyway.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
More information about the cfe-commits
mailing list