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