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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:766
+      [File, Code, Params, Reply = std::move(Reply),
+       this](llvm::Expected<FileEdits> Edits) mutable {
+        if (!Edits)
----------------
NIT: is capture of `this` redundant here?

I could be missing something, though.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:334
+
     SourceLocation Loc = getBeginningOfIdentifier(
         Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts());
----------------
Why not do this before running rename? This would allow to:
1. return result without doing the actual (expensive) rename if there's no identifier under the cursor
2. reduce nesting of the (now large) large piece of code that runs rename:
```
auto Loc = getBeginningOfIdentifier(...);
auto Range = ...;
if (!Range)
  return llvm::None;
if (!CrossFileRename)
  return *Range; // FIXME: assume cross-file rename always succeeds
// do the rename...
```


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:353
       return CB(InpAST.takeError());
-    auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index);
-    if (!Changes)
-      return CB(Changes.takeError());
+    RenameInputs RInputs;
+    RInputs.MainFileCode = InpAST->Inputs.Contents;
----------------
Could we get a helper function (or constructor) to keep the call sites as simple as they used to be?
`RenameInputs{Contents, File, NewName, Pos,...}` should also work just fine and I don't think it hurts readability, since all the parameters here are named the same as the fields and most of them have different types.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:360
+    RInputs.Index = Index;
+    RInputs.VFS = FSProvider.getFileSystem();
+    RInputs.GetDirtyBuffer = GetDirtyBuffer;
----------------
This can always be obtained from the AST, right?
Why do we need it separately?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370
+      for (auto &E : *Edits) {
+        if (auto Err = reformatEdit(E.getValue(), Style))
+          elog("Failed to format replacements: {0}", std::move(Err));
----------------
Maybe use `llvm::joinErrors` to combine multiple failures into a single error?
Should simplify the code.


================
Comment at: clang-tools-extra/clangd/SourceCode.h:226
 };
+using FileEdits = llvm::StringMap<Edit>;
 
----------------
We should document this typedef, otherwise it does not provide much value.
A doc comment from `ApplyEdits` should fit just nicely here.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:71
+                                          bool CrossFile) {
+  // Filter out symbols that are unsupported in both rename mode.
   if (llvm::isa<NamespaceDecl>(&RenameDecl))
----------------
NIT: in both rename **modes**


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:23
 
+using DirtyBufferGetter =
+    std::function<llvm::Optional<std::string>(PathRef Path)>;
----------------
ilya-biryukov wrote:
> Could you document what does it mean for this function to return `llvm::None`?
> I assume we read the contents from disk instead.
> 
> Also worth documenting what should be returned for `MainFilePath`? `llvm::None`? same value as `MainFileCode`?
What does it return for `MainFilePath`? `llvm::None`? `MainFileCode`?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:33
+  llvm::StringRef MainFilePath;
+  llvm::StringRef MainFileCode;
+
----------------
`MainFileCode` can be obtained from the AST. Why not do it?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:40
+  // When set, used by the rename to get file content for all rename-related
+  // files (except the main file).
+  // If there is no corresponding dirty buffer, we will use the file content
----------------
Why is `MainFile` special? Can it also be handled with the GetDirtyBuffer?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:49
+/// in another file (per the index).
+llvm::Expected<FileEdits> rename(ParsedAST &AST, const RenameInputs &RInputs);
 
----------------
Why isn't `AST` part of the `RenameInputs`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263