[PATCH] D73610: [clangd] Replace raw lexer code with token buffer in prepare rename.

[Sam McCall via Phabricator via cfe-commits]

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