[PATCH] D63126: [clangd] Implement "prepareRename"

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:393
         }}}};
+  if (Params.capabilities.RenamePrepareSupport)
+    Result["renameProvider"] = llvm::json::Object{{"prepareProvider", true}};
----------------
why set this only if the client advertised support?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:295
+    auto &AST = InpAST->AST;
+    // Return null if the "rename" is not valid at the position.
+    auto Changes = renameWithinFile(AST, File, Pos, "dummy", Index);
----------------
move this comment to at/in the !Changes block?


================
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) {
----------------
High level comment indicating why we use rename to implement preparerename?
(i.e. why isn't it too expensive)


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:298
+    if (!Changes) {
+      llvm::consumeError(Changes.takeError());
+      return CB(llvm::None);
----------------
I thought the point of supporting this was to get errors to the user sooner. That can't happen if we throw away the error message...


================
Comment at: clang-tools-extra/clangd/Protocol.h:412
+  /// before execution.
+  bool RenamePrepareSupport = false;
 };
----------------
(i think we shouldn't need to parse this at all)


================
Comment at: clang-tools-extra/clangd/test/prepare-rename.test:2
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument": {"rename": {"dynamicRegistration": true, "prepareSupport": true}}},"trace":"off"}}
+# CHECK:      "renameProvider": {
----------------
inline this test into rename.test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63126