[PATCH] D75176: [clangd] Get rid of getBeginningOfIdentifier helper

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hooray!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:537
 
+  // We prefer the identifiers if any, otherwise make use of the first token.
+  SourceLocation SourceLocationBeg = TokensAroundCursor.front().location();
----------------
first -> last I think, better explained as rightmost. See SelectionTree::create().


================
Comment at: clang-tools-extra/clangd/Hover.cpp:537
 
+  // We prefer the identifiers if any, otherwise make use of the first token.
+  SourceLocation SourceLocationBeg = TokensAroundCursor.front().location();
----------------
sammccall wrote:
> first -> last I think, better explained as rightmost. See SelectionTree::create().
actually, if this is really only used for macros and auto, you could pull out those specific cases (define AutoLoc and IdentifierLoc and initialize them by looping over the touching tokens). Seems a bit simpler/more direct/less risk of reuse for other things?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:540
+  for (const auto &Tok : TokensAroundCursor) {
+    if (Tok.kind() != tok::identifier)
+      continue;
----------------
This deserves a comment, it's a different strategy than SelectionTree that should yield the same result.

("In general we prefer the touching token that works over the one that doesn't, see SelectionTree::create(). This location is used only for triggering on macros and auto, so simply choosing the lone identifier-or-keyword token is equivalent")


================
Comment at: clang-tools-extra/clangd/Hover.cpp:540
+  for (const auto &Tok : TokensAroundCursor) {
+    if (Tok.kind() != tok::identifier)
+      continue;
----------------
sammccall wrote:
> This deserves a comment, it's a different strategy than SelectionTree that should yield the same result.
> 
> ("In general we prefer the touching token that works over the one that doesn't, see SelectionTree::create(). This location is used only for triggering on macros and auto, so simply choosing the lone identifier-or-keyword token is equivalent")
you also need to check for keyword (or specifically kw auto)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75176