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 = 100; + if (auto ID = getSymbolID(RenameDecl)) ---------------- Why do we need to limit the number of references in the first place? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:223 + + // Helper. + auto ToRange = [](const SymbolLocation &L) { ---------------- Maybe remove this comment? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:224 + // Helper. + auto ToRange = [](const SymbolLocation &L) { + Range R; ---------------- Why not extract this as a helper function? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:233 + llvm::StringMap<std::vector<Range>> + AffectedFiles; // absolute file path => rename ocurrences in that file. + Index->refs(RQuest, [&](const Ref &R) { ---------------- NIT: maybe put this comment on the line before the declaration? Should lead to better formatting Also, this comment would be most useful on the function declaration. Maybe move it there? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274 +llvm::Expected<FileEdits> +renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath, + llvm::StringRef NewName, const SymbolIndex *Index, ---------------- NIT: Maybe call it `renameWithIndex` instead? It should capture what this function is doing better... But up to you ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:289 + + // The cross-file rename is purely based on the index, as we don't want to + // build all ASTs for affected files, which may cause a performance hit. ---------------- Maybe move this comment to the function itself? It definitely does a great job of capturing what this function is doing and what the trade-offs are. ================ Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:277 + // - file on VFS for bar.cc; + Annotations MainCode("class [[Fo^o]] {};"); + auto MainFilePath = testPath("main.cc"); ---------------- hokein wrote: > ilya-biryukov wrote: > > Thinking whether we can encode these transformations in a nicer way? > > If we could convert the contents dirty buffer and replacements to something > > like: > > ``` > > class [[Foo |-> newName]] {}; > > ``` > > Just a proposal, maybe there's a better syntax here. > > > > We can easily match strings instead of matching ranges in the tests. This > > has the advantage of having textual diffs in case something goes wrong - > > much more pleasant than looking at the offsets in the ranges. > > > > WDYT? > I agree textual diff would give a more friendly results when there are > failures in the tests. > > I don't have a better way to encode the transformation in the annotation > code, I think a better way maybe to use a hard-coded new name, and applying > the actual replacements on the testing code, and verify the the text diffs. > > If we want to do this change, I'd prefer to do it in a followup, since it > would change the existing testcases as well. What do you think? > Maybe I'm overthinking it... For rename having just the range is probably enough. ================ Comment at: clang-tools-extra/clangd/unittests/SyncAPI.cpp:103 + Server.rename(File, Pos, NewName, /*WantFormat=*/true, + /*DirtyBuffaer*/ nullptr, capture(Result)); return std::move(*Result); ---------------- s/DirtyBuffaer/DirtyBuffer also use `/*DirtryBuffer=*/` to be consistent with `/*WantFormat=*/` from the previous line 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