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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks, this is in a good shape!
The only concern I have is the racy way to get dirty buffers, please see the corresponding comment.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:764
+      /*WantFormat=*/true,
+      [this](PathRef File) { return DraftMgr.getDraft(File); },
+      [File, Params, Reply = std::move(Reply),
----------------
We should probably get a snapshot of all dirty buffers here instead.
A racy  way (rename is run on a separate thread, new files updates might come in in the meantime) to get contents of the file looks like a bad idea, this will lead to hard-to-debug failures...

Note that `ClangdServer` also has access to all contents of all the files, they are stored in `TUScheduler`, so we don't need to pass `GetDirtyBuffer` callback up until the final run of `rename`


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:317
+    if (!Range)
+      // Return null if the "rename" is not valid at the position.
+      return CB(llvm::None);
----------------
NIT: comment could be shortened to 
```
/// "rename" is not valid at the position.
```
Or even removed.
Both would allow saving a line (if we put the comment on the same line as `return`


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:320
+    if (CrossFileRename)
+      return CB(*Range); // FIXME: assueme cross-file rename always succeeds.
+
----------------
NIT: s/assueme/assume

also, in FIXME we normally give the final state, so it should probably be 
```
/// FIXME: do not assume cross-file rename always succeeds
```




================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:340
 void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
-                          bool WantFormat, Callback<std::vector<TextEdit>> CB) {
+                          bool WantFormat, DirtyBufferGetter GetDirtyBuffer,
+                          Callback<FileEdits> CB) {
----------------
`ClangdServer` can obtain the dirty buffers via `TUScheduler::getContents`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69263