[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/ClangdServer.cpp:370
+      for (auto &E : *Edits) {
+        if (auto Err = reformatEdit(E.getValue(), Style))
+          elog("Failed to format replacements: {0}", std::move(Err));
----------------
hokein wrote:
> 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));
>    }
> }
> ```
Works nicely if you start with `Error::success`
```
auto Err = Error::success();
for (...) {
  Err = llvm::joinErrors(reformatEdit(E.getValue(), Style));
}
if (Err)
  return CB(std::move(Err));
```


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:360
+                         GetDirtyBuffer};
+    auto Edits = clangd::rename(RInputs);
+    if (!Edits)
----------------
NIT: inlining `RInputs` should save quite a few lines of code


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:91
+  if (!CrossFile) {
+    // Within-file rename.
+    auto &ASTCtx = RenameDecl.getASTContext();
----------------
NIT: this comment is redundant, previous line says the same thing


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+    if (!Index)
+      return NoIndexProvided;
+
----------------
Why isn't this a scope enum in the first place?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128
+
+  // A whitelist of renamable symbols.
+  if (llvm::isa<CXXRecordDecl>(RenameDecl))
----------------
Why not a blacklist? I'd expect us to blacklist:
- things with custom names (operators, constructors, etc)
- virtual methods

Probably some things are missing from the list, but fundamentally most that have names are pretty similar, right?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+  if (const auto *S = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl)) {
+    // FIXME: renaming virtual methods requires to rename all overridens in
+    // subclasses, which our index doesn't support yet.
----------------
Isn't the same true for local rename?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:221
+  if (!Index)
+    return {};
+  RefsRequest RQuest;
----------------
This behavior is very surprising... Obviously, returning empty results is incorrect and the callers have to handle the no-index case in a custom manner.

Maybe accept only non-null index and handle in the caller?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:235
+  });
+  return AffectedFiles;
+}
----------------
Again, it looks like the function returns incorrect results and the callers should actually handle the case where `SymbolID` cannot be obtained in a custom manner.

Maybe accept a `SymbolID` as a parameter instead of `NamedDecl*` and the callers handle this case gracefully?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+    // !positionToOffset is O(N), it is okay at the moment since we only
+    // process at most 100 references.
+    auto RangeOffset = toRangeOffset(Occurrence, InitialCode);
----------------
This is not true anymore, right?
We should probably try getting to `O(N)` to avoid slowing things down

Could be a FIXME for now, but have to fix it soon.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:33
+  llvm::StringRef MainFilePath;
+  llvm::StringRef MainFileCode;
+
----------------
hokein wrote:
> 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`.
`SourceManager` has access to the input buffer.
`StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());`


================
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
----------------
hokein wrote:
> 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). 
Does that mean we have three states of files that rename should be aware of?
- Dirty buffer
- Main file contents
- Contents on disk

That definitely looks very complicated... Is there a chance we could abstract it away and just have a single source of truth for file contents?
I am thinking of a function that handles getting the file buffer by name, so we have all the complicated logic in one place. Would look something like:
```
std::function<std::string(PathRef /*Path*/)> GetFileContents;
```
Implementation could try to do the following in order:
1. try to get contents of the main file.
2. if failed, get contents from the dirty buffers.
3. if failed, get contents from disk.

Could be a helper function accepting `RenameInputs`, but important to use it everywhere we get the file contents.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263