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