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

[Haojian Wu via Phabricator via cfe-commits]

hokein 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)
----------------
ilya-biryukov wrote:
> NIT: is capture of `this` redundant here?
> 
> I could be missing something, though.
it is not redundant, we access the `this->DraftMgr` in the lambda body.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:334
+
     SourceLocation Loc = getBeginningOfIdentifier(
         Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts());
----------------
ilya-biryukov wrote:
> 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...
> ```
Good point.


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


================
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));
----------------
ilya-biryukov wrote:
> Maybe use `llvm::joinErrors` to combine multiple failures into a single error?
> Should simplify the code.
I don't see using `llvm::joinErrors` can simplify the code here, `joinErrors` accepts two `Error` objects, and there is no way to create an empty Error object, we may end up with more code like:

```
llvm::Optional<llvm::Error> Err;
for (auto &E : *Edits) {
   if (auto E = reformatEdit(E.getValue(), Style)) {
      if (!Err) Err = std::move(E);
      else Err = joinErrors(std::move(*Err), std::move(Err));
   }
}
```


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:33
+  llvm::StringRef MainFilePath;
+  llvm::StringRef MainFileCode;
+
----------------
ilya-biryukov wrote:
> `MainFileCode` can be obtained from the AST. Why not do it?
`ParsedAST` doesn't provide such API to get the file code, I assume you mean `InputsAndAST`? The `MainFileCode` is from `InputsAndAST`.


================
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
----------------
ilya-biryukov wrote:
> Why is `MainFile` special? Can it also be handled with the GetDirtyBuffer?
I believe we should use the content from `InputsAndAST`, which is corresponding to the main AST (the content from `GetDiryBuffer` may not be reflected to the AST). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263