hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:322 + if (!TouchingIdentifier) + return CB(llvm::None); // no rename on non-identifiers. + ---------------- sammccall wrote: > Agree that this behavior is OK per the spec, but do we actually prefer it/why > the change? > > Seems like clients are marginally more likely to handle null > incorrectly/silently, and we give up control over the message. > Agree that this behavior is OK per the spec, but do we actually prefer it/why > the change? I checked with VSCode, it seems fine (`the element can't be renamed`), and YCM (but it doesn't support prepare rename yet). Are you suggesting that we return our message here? ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:324 + + auto Range = getTokenRange(AST.getSourceManager(), AST.getLangOpts(), + TouchingIdentifier->location()); ---------------- sammccall wrote: > this runs the lexer to compute token boundaries. We actually already have > them: syntax::Token has endLocation/length removed it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73610/new/ https://reviews.llvm.org/D73610 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
