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

This revision was automatically updated to reflect the committed changes. Closed by commit rG852bafae2bb4: [clangd] Implement cross-file rename. (authored by hokein). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/

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

merge_guards_bot added a comment. Build result: pass - 60175 tests passed, 0 failed and 732 were skipped. Log files: console-log.txt , CMakeCache.txt

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

hokein updated this revision to Diff 230068. hokein marked an inline comment as done. hokein added a comment. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/ https://reviews.llvm.org/D69263 Files:

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

hokein marked an inline comment as done. hokein added a comment. In 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

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

hokein updated this revision to Diff 229815. hokein marked an inline comment as done. hokein added a comment. rebase and call getMacroArgExpandedLocation in prepareRename. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. 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

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

hokein updated this revision to Diff 228294. hokein marked 13 inline comments as done. hokein added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/ https://reviews.llvm.org/D69263 Files:

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

hokein added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:345 + SourceLocation SourceLocationBeg = + SM.getMacroArgExpandedLocation(getBeginningOfIdentifier( + RInputs.Pos, SM, AST.getASTContext().getLangOpts()));

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:342 + // A snapshot of all file dirty buffers. + llvm::StringMap SnapShot = WorkScheduler.getAllFileContents(); auto Action = [File = File.str(), NewName = NewName.str(), Pos,

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

hokein updated this revision to Diff 228213. hokein marked 5 inline comments as done. hokein added a comment. Herald added a subscriber: javed.absar. - get dirty buffer in clangdServer layer; - save a snpatshot for all dirty buffer; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:764 + /*WantFormat=*/true, + [this](PathRef File) { return DraftMgr.getDraft(File); }, + [File, Params, Reply = std::move(Reply), ilya-biryukov wrote: > We

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

ilya-biryukov added a comment. Thanks, this is in a good shape! The only concern I have is the racy way to get dirty buffers, please see the corresponding comment. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:764 + /*WantFormat=*/true, + [this](PathRef

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

merge_guards_bot added a comment. Build result: fail - 59861 tests passed, 21 failed and 763 were skipped. failed: lld.ELF/linkerscript/filename-spec.s failed: Clang.Index/index-module-with-vfs.m failed: Clang.Modules/double-quotes.m failed:

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

hokein updated this revision to Diff 228028. hokein marked 5 inline comments as done. hokein added a comment. - use blacklist - change to enum scope Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/ https://reviews.llvm.org/D69263 Files:

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

hokein added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107 +if (!Index) + return NoIndexProvided; + ilya-biryukov wrote: > hokein wrote: > > ilya-biryukov wrote: > > > Why isn't this a scope enum in the first place? > >

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107 +if (!Index) + return NoIndexProvided; + hokein wrote: > ilya-biryukov wrote: > > Why isn't this a scope enum in the first place? > this is tricky, the

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

merge_guards_bot added a comment. Build result: fail - 59795 tests passed, 21 failed and 762 were skipped. failed: lld.ELF/linkerscript/filename-spec.s failed: Clang.Index/index-module-with-vfs.m failed: Clang.Modules/double-quotes.m failed:

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

hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370 + for (auto : *Edits) { +if (auto Err = reformatEdit(E.getValue(), Style)) + elog("Failed to format replacements: {0}", std::move(Err)); ilya-biryukov

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

hokein updated this revision to Diff 227860. hokein marked 13 inline comments as done. hokein added a comment. address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/ https://reviews.llvm.org/D69263 Files:

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370 + for (auto : *Edits) { +if (auto Err = reformatEdit(E.getValue(), Style)) + elog("Failed to format replacements: {0}", std::move(Err)); hokein

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

merge_guards_bot added a comment. Build result: pass - 59816 tests passed, 0 failed and 762 were skipped. Log files: console-log.txt , CMakeCache.txt

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

hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:766 + [File, Code, Params, Reply = std::move(Reply), + this](llvm::Expected Edits) mutable { +if (!Edits) ilya-biryukov wrote: > NIT: is capture of `this`

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

hokein updated this revision to Diff 227700. hokein marked 13 inline comments as done. hokein added a comment. address comments: - use VFS from AST; - simplify the interfaces; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:766 + [File, Code, Params, Reply = std::move(Reply), + this](llvm::Expected Edits) mutable { +if (!Edits) NIT: is capture of `this` redundant here?

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

merge_guards_bot added a comment. Build result: pass - 59816 tests passed, 0 failed and 762 were skipped. Log files: console-log.txt , CMakeCache.txt

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

hokein updated this revision to Diff 227663. hokein marked 4 inline comments as done. hokein added a comment. Add comments for DirtyBufferGetter. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/ https://reviews.llvm.org/D69263 Files:

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

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274 +llvm::Expected +renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath, + llvm::StringRef

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

merge_guards_bot added a comment. Build result: pass - 59816 tests passed, 0 failed and 762 were skipped. Log files: console-log.txt , CMakeCache.txt

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

hokein updated this revision to Diff 227278. hokein marked 8 inline comments as done. hokein added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/ https://reviews.llvm.org/D69263 Files:

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

hokein added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:219 + // us whether the ref results is completed. + RQuest.Limit = 100; + if (auto ID = getSymbolID(RenameDecl)) ilya-biryukov wrote: > Why do we need to limit the number

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

ilya-biryukov added a comment. Thanks, mostly LG! The only important comment I have left is about limiting the number of references. Others are NITs. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:219 + // us whether the ref results is completed. + RQuest.Limit =

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

merge_guards_bot added a comment. Build result: pass - 59701 tests passed, 0 failed and 763 were skipped. Log files: cmake-log.txt , ninja_check_all-log.txt

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

hokein updated this revision to Diff 226641. hokein marked 3 inline comments as done. hokein added a comment. address comments: - make the command-line flag hidden; - use the gtesting matcher; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

hokein added inline comments. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:269 +cat(Features), +desc("Enable cross-file rename feature."), +init(false), ilya-biryukov wrote: > Could you document that the feature is highly experimental and

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:269 +cat(Features), +desc("Enable cross-file rename feature."), +init(false), Could you document that the feature is highly experimental and may lead to

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

merge_guards_bot added a comment. Build result: fail - 59520 tests passed, 1 failed and 763 were skipped. failed: Clangd.Clangd/rename.test Log files: cmake-log.txt , ninja_check_all-log.txt

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

hokein added a comment. In D69263#1718525 , @ilya-biryukov wrote: > Not sure that holds. What if the file in question is being rebuilt right now? > We do not wait until all ASTs are built (and the dynamic index gets the new > results). > Open files

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

hokein updated this revision to Diff 226414. hokein marked 5 inline comments as done. hokein added a comment. - simplify the code, addressing comments; - add unittests; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:119 +llvm::Optional +renameableOutsideFile(const NamedDecl , const SymbolIndex *Index) { hokein wrote: > ilya-biryukov wrote: > > So `llvm::None` means we do not

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

merge_guards_bot added a comment. Build result: fail - 59518 tests passed, 1 failed and 763 were skipped. failed: Clangd.Clangd/rename.test Log files: cmake-log.txt , ninja_check_all-log.txt

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

merge_guards_bot added a comment. Build result: fail - 59475 tests passed, 2 failed and 805 were skipped. failed: Clangd.Clangd/rename.test failed: LLVM.tools/llvm-ar/mri-utf8.test Log files: cmake-log.txt ,

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

hokein updated this revision to Diff 226137. hokein marked 13 inline comments as done. hokein added a comment. - address comments; - add more FIXMEs; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/ https://reviews.llvm.org/D69263 Files:

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

hokein added a comment. In D69263#1718525 , @ilya-biryukov wrote: > In D69263#1717985 , @hokein wrote: > > > Thinking more about this -- we have a dynamic index (for all opened files) > > which is overlaid on a

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

ilya-biryukov added a comment. In D69263#1717985 , @hokein wrote: > Thinking more about this -- we have a dynamic index (for all opened files) > which is overlaid on a static index (which is a background index in > open-source world), so for all

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

hokein marked an inline comment as done. hokein added a comment. Thanks for the comments. In D69263#1716760 , @ilya-biryukov wrote: > Another important concern is surfacing errors to the users: silently dropping > ranges for stale files is definitely

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

ilya-biryukov added a comment. Thanks for implementing this. I think we could split it into multiple changes to make understanding it easier, see inline comments, I've tried to point out the places I find relevant. Would definitely be nice to have some unit-tests for this. Another important

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

merge_guards_bot added a comment. Build result: fail - 33608 tests passed, 1 failed and 462 were skipped. failed: LLVM.tools/llvm-ar/mri-utf8.test Log files: cmake-log.txt , ninja_check_all-log.txt

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

hokein added a comment. tests are missing, but the patch should be sufficient for initial review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69263/new/ https://reviews.llvm.org/D69263 ___

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

hokein created this revision. hokein added reviewers: ilya-biryukov, sammccall. Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. This is the initial version. The cross-file rename is purely based on the index (plus a text-match