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

[Simon Marchi via Phabricator via cfe-commits]

simark marked 3 inline comments as done.
simark added inline comments.


================
Comment at: clangd/DraftStore.cpp:61
+      const Position &Start = Change.range->start;
+      size_t StartIndex = positionToOffset(Contents, Start);
+      if (StartIndex > Contents.length()) {
----------------
ilya-biryukov wrote:
> `positionToOffset` always truncates to size of contents. It has to return `Expected<int>` too here.
Indeed, it would be better.


================
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 "
----------------
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.


================
Comment at: clangd/DraftStore.cpp:97
+
+      NewContents.reserve(StartIndex + Change.text.length() +
+                          (Contents.length() - EndIndex));
----------------
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.


================
Comment at: clangd/DraftStore.cpp:103
+
+      if (EndIndex < Contents.length())
+        NewContents += Contents.substr(EndIndex);
----------------
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().


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272