sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:452 + for (int Pos : MatchedIndex) + Mapped.push_back(Lexed[Pos]); + return MatchedCB(std::move(Mapped)); ---------------- hokein wrote: > sammccall wrote: > > if we're actually evaluating all ranges, can we pass the index array (by > > reference), use it to evaluate scores, and only copy ranges for the winner? > we could use the index array to evaluate the scores, but it would make the > cost API signature a bit weird, like `size_t > renameRangeAdjustmentCost(ArrayRef<Range> Indexed, ArrayRef<Range> Lexed, > ArrayRef<size_t> MatchedLexedIndex);` It's not really an API right, just a helper function exposed for testing? I don't think this is a a problem. ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:359 } - auto RenameEdit = - buildRenameEdit(FilePath, *AffectedFileCode, - std::move(FileAndOccurrences.second), NewName); + auto RenameCandidates = + adjustRenameRanges(*AffectedFileCode, RenameDecl.getNameAsString(), ---------------- nit: these are not candidates, they are `RenameRanges` or similar ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:364 + if (!RenameCandidates) { + return llvm::make_error<llvm::StringError>( + llvm::formatv("Index results don't match the content of file {0} " ---------------- This is worth a comment (because the error message returned describes a condition that we usually recover from) ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:569 +// possible +llvm::Expected<std::vector<Range>> +adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier, ---------------- why returning Expected rather than Optional here - what do we want to do with the message? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:589 + llvm::inconvertibleErrorCode(), + llvm::formatv("The number of lexed occurrences is less than indexed " + "occurrences")); ---------------- This message isn't meaningful outside this TU - it should be a vlog, or easier to understand ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:593 + if (std::includes(Indexed.begin(), Indexed.end(), Lexed.begin(), Lexed.end())) + return std::vector<Range>{Indexed.begin(), Indexed.end()}; + ---------------- return Indexed.vec() ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:618 + llvm::inconvertibleErrorCode(), + llvm::formatv("The best near miss is not distinct")); + if (Best.empty()) ---------------- distinct -> unique ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:622 + llvm::inconvertibleErrorCode(), + llvm::formatv("Didn't found a near miss")); + return Best; ---------------- found -> find ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:622 + llvm::inconvertibleErrorCode(), + llvm::formatv("Didn't found a near miss")); + return Best; ---------------- sammccall wrote: > found -> find (again, these error messages are not useful to the user, as-is) ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:82 +/// REQUIRED: Indexed and Mapped are sorted, and have the same size. +size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed, + ArrayRef<Range> Mapped); ---------------- also exposed for testing only? ================ Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:866 + { + // both line and column are changed, not a near miss. + R"cpp( ---------------- can you make the new line non-empty (add a comment) and change int->double instead of adding whitespace? Was hard to see what's going on here Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70594/new/ https://reviews.llvm.org/D70594 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits