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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
