[PATCH] D70441: [clangd] Speed up when building rename edit.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.h:54
+/// Use of UTF-16 may be overridden by kCurrentOffsetEncoding.
+llvm::Expected<size_t> byteOffset(StringRef LineCode, int LSPCharacter);
+
----------------
ilya-biryukov wrote:
> How this is different from `positionToOffset` with `Pos.line = 0` and `Pos.column = LSPCharacter`?
> Why do we need an extra function?
removed now, not needed.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:441
 
+llvm::Expected<Edit> buildRenameEdit(llvm::StringRef InitialCode,
+                                     const std::vector<Range> &Occurrences,
----------------
ilya-biryukov wrote:
> We are aiming to convert all ranges in `O(n)` instead of `O(n^2)`, right?
> I think this could be simpler and also have guaranteed performance even for long lines if we do it slightly differently. WDYT?
> 
> ```
> assert(llvm::is_sorted(Occurences));
> // These two always correspond to the same position.
> Position LastPos = Position{0, 0};
> size_t LastOffset = 0;
> 
> std::vector<pair<size_t, size_t>> Offsets;
> for (auto R : Occurences) {
>   Position ShiftedStart = R.start - LastPos;
>   size_t ShiftedStartOffset = positionToOffset(ShiftedStart, Code.substr(LastOffset);
> 
>   LastPos = ShiftedStart;
>   LastOffset  = ShiftedStartOffset;
>   // Do the same for the end position.
>   // ...
> }
> 
> // Now we can easily replacements.
> // ...
> ```
agree, this is smart. note that we need to pay the cost of sorting, but I think it is totally ok as the number is relatively small.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70441/new/

https://reviews.llvm.org/D70441