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

[Ilya Biryukov via Phabricator via cfe-commits]

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