[PATCH] D69263: [clangd] Implement cross-file rename.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added a comment.

In D69263#1738289 <https://reviews.llvm.org/D69263#1738289>, @ilya-biryukov wrote:

> LGTM.
>
> It's probably worth collecting a list of things we need to fix before enabling cross-file rename and putting it somewhere (a GitHub issue, maybe?)
>  Important things that immediately come to mind:
>
> - add range-patching heuristics, measure how good they are
> - avoid `O(N^2)` when computing edits (converting from positions to offsets)
> - handle implicit references from the index
>
>   There are definitely more.


Thanks, filed at issues#193.



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:345
+  SourceLocation SourceLocationBeg =
+      SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+          RInputs.Pos, SM, AST.getASTContext().getLangOpts()));
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Why is this different from `prepareRename`, which does not call `getMacroArgExpandedLocation`?
> > > 
> > I didn't change it in this patch, but you raise a good point, `prepareRename` should call `getMacroArgExpandedLocation`.
> Yep, makes sense to change it later. Could you put a FIXME, so that we don't forget about it?
Done, fixed this in this patch, it is trivial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263