[PATCH] D44272: [clangd] Support incremental document syncing

[Ilya Biryukov via Phabricator via cfe-commits]

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 `replyError`
> `textDocument/didChange` is a jsonrpc notification, not request, so we can't send back an error.
Right, my mistake. Sorry for the confusion.


================
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 "
----------------
simark wrote:
> ilya-biryukov wrote:
> > 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);
> > }
> > ```
> It makes sense, but I think we can achieve the same result by just tweaking the current code. I don't think a separate function is really needed here.
Sure, tweaking the function is also ok. Have to be careful to not accidentally update the old `Contents` before all changes are applied, but no problems other than that.


================
Comment at: clangd/DraftStore.cpp:97
+
+      NewContents.reserve(StartIndex + Change.text.length() +
+                          (Contents.length() - EndIndex));
----------------
simark wrote:
> ilya-biryukov wrote:
> > 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`
> I saw it as `LengthOfTheSliceFromTheOriginalThatWeKeepBefore + LengthOfNewStuff + LengthOfTheSliceFromTheOriginalThatWeKeepAfter`.  But I don't mind changing it.
Ha, that also makes sense to me, thanks for the explanation. And even mirrors the code better.


================
Comment at: clangd/DraftStore.cpp:103
+
+      if (EndIndex < Contents.length())
+        NewContents += Contents.substr(EndIndex);
----------------
simark wrote:
> ilya-biryukov wrote:
> > This check seems unnecessary, we've already checked for range errors. Maybe remove it?
> Note that this check is not equivalent to the previous
> 
>     if (EndIndex > Contents.length())
> 
> for the case where `EndIndex == Contents.length()`. In our case, it's possible (and valid) for EndIndex to be equal to Contents.length(), when referring to the end of the document (past the last character).  I thought that doing `Contents.substr(Contents.length())` would be throw `std::out_of_range` or be undefined behavior, but now that I read it correctly:
> 
>     @throw  std::out_of_range  If __pos > size().
> 
> So it looks like it would fine to have pos == Contents.length().
 Right, the `substr` will simply return an empty string. An extra check is not really necessary.


================
Comment at: clangd/DraftStore.cpp:106
+    } else {
+      NewContents = Change.text;
+    }
----------------
simark wrote:
> ilya-biryukov wrote:
> > 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");
> > }
> > ```
> > 
> I'd say it is unlikely and there's probably no reason to do it, but the way the data structure is allows it.
I would be very surprised to see a request that mixes both, even though the data structure allows it. It's up to you if you want to change the behaviour or not.
If we keep the code this way, maybe first handle the full content update? It would make the code somewhat easier to read and reduce nesting (see the relevant bit of [[https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code|LLVM style guide]]):
```
for (auto &Change : Changes) {
  if (!Change.range) {
    Contents = std::move(Change.Text);
    continue;
  }

  // Handle incremental change
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272