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