[PATCH] D70594: [clangd] Implement range patching heuristics for cross-file rename.

[Sam McCall via Phabricator via cfe-commits]

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