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