sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:393
}}}};
+ if (Params.capabilities.RenamePrepareSupport)
+ Result["renameProvider"] = llvm::json::Object{{"prepareProvider", true}};
----------------
hokein wrote:
> sammccall wrote:
> > why set this only if the client advertised support?
> The LSP says
> ```
> /**
> * The server provides rename support. RenameOptions may only be
> * specified if the client states that it supports
> * `prepareSupport` in its initial `initialize` request.
> */
> renameProvider?: boolean | RenameOptions;
> ```
>
> so we only send `RenameOptions` when the client declares it supports
> prepareRename.
Ah, OK. this should be documented.
Could we put this logic above instead, for less data structure wrangling?
```
llvm::json::Value RenameProvider = llvm::json::Object{{"prepareProvider",
true}};
if (!Params.capabilities.RenamePrepareSupport) // Only boolean allowed per LSP
RenameProvider = true;
...
{"renameProvider", std::move(RenameProvider)}
```
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:581
+ Params.textDocument.uri.file(), Params.position,
+ Bind(
+ [](decltype(Reply) Reply, llvm::Expected<llvm::Optional<Range>> R) {
----------------
this appears to be the same as just passing std::move(Reply) to preparerename
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:298
+ if (!Changes) {
+ // LSP clients may emit a hard-coded error message (e.g. "can't be
+ // renamed" in VSCode) if we returns null, we use the interl error
message
----------------
There's some context missing here - you're explaining *why* we're going against
LSP, but you haven't explained that we're doing so!
"LSP says to return null on failure, but that will result in a generic failure
message. If we send an LSP error response, clients can surface the message to
users (VSCode does)"
("will" not "may", because there's no way it can result in a detailed message)
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:296
+ // Return null if the "rename" is not valid at the position.
+ auto Changes = renameWithinFile(AST, File, Pos, "dummy", Index);
+ if (!Changes) {
----------------
sammccall wrote:
> High level comment indicating why we use rename to implement preparerename?
> (i.e. why isn't it too expensive)
can you say something like "Performing the rename isn't substantially more
expensive than an AST-based check, so we just rename and throw away the
results. We may have to revisit this when we support cross-file rename"
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63126/new/
https://reviews.llvm.org/D63126
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
