[PATCH] D88634: [clangd] Extend the rename API.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:416
+    auto Results = clangd::rename(
+        {Pos, "dummy", InpAST->AST, File,
+         RenameOpts.AllowCrossFile ? &EmptyIndex : Index, RenameOpts});
----------------
sammccall wrote:
> we're now returning the "dummy" string to the caller, so we should document it somewhere (or ideally just make it the empty string and document that)
make it empty string, and added document.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+  Range R;
+  FileEdits Edits;
+};
----------------
sammccall wrote:
> It's slightly awkward to have the half-populated state (may or may not contain cross-file results, can't tell without the query).
> 
> I'd consider redundantly including `Edit LocalChanges` and `FileEdits GlobalChanges` with the former always set and the latter empty when returned from preparerename.
> It's slightly awkward to have the half-populated state (may or may not contain cross-file results, can't tell without the query).

I feel this is not too bad, the query is quite trivial, just `Edits.size() > 1`.

> I'd consider redundantly including Edit LocalChanges and FileEdits GlobalChanges with the former always set and the latter empty when returned from preparerename.

This change seems nice to get changes for main-file, but I think `GlobalChanges` should include LocalChanges (otherwise, client side needs to combine these two when applying the final rename edits)? then we can't leave the later empty while keeping the former set.

Happy to do the change, but it looks like we don't have usage of `LocalChanges` so far. In prepareRename, we want main-file occurrences, `rename` will always return them regardless of single-file or cross-file rename, so I think `Edits` is efficient. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88634/new/

https://reviews.llvm.org/D88634