sammccall added a comment. Nice, thanks!
================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:322 + if (!TouchingIdentifier) + return CB(llvm::None); // no rename on non-identifiers. + ---------------- 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. ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:324 + + auto Range = getTokenRange(AST.getSourceManager(), AST.getLangOpts(), + TouchingIdentifier->location()); ---------------- this runs the lexer to compute token boundaries. We actually already have them: syntax::Token has endLocation/length ================ Comment at: clang-tools-extra/clangd/SourceCode.h:302 +/// Returns the identifier token that touches the given \p Pos +/// Redturns nullptr if there is none. ---------------- I don't think this function improves things vs keeping it inline: - wrapping clang functions to adapt them to our data types doesn't seem sustainable - it's hard to draw the line, but this one neither hides logic (saves 2 trivial lines) nor is very commonly used (yet) - this breaks a (minor) error-handling path: an invalid range is no longer reported to the client as an error 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
