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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
