[clangd] Get rid of getBeginningOfIdentifier helper

[sammccall]

Hooray!

clang-tools-extra/clangd/Hover.cpp

537

first -> last I think, better explained as rightmost. See SelectionTree::create().

537

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?

540

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")

540

you also need to check for keyword (or specifically kw auto)

[kadircet]

• Only use special locations for deducedtype and macros.

[sammccall]

Did you want to delete the lexer check in getDeducedType or is that another patch?

clang-tools-extra/clangd/Hover.cpp

532

again, I'd stick with "touching" to match syntax::tokens

551

nit: we don't tend to use SLoc to refer to SourceLocations outside SourceManager, or ID to refer to identifiers.
IdentLoc, AutoLoc?

clang-tools-extra/clangd/Hover.cpp

 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST) {
   return llvm::None;
 }
 } // namespace
 
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index) {
   const SourceManager &SM = AST.getSourceManager();
-  llvm::Optional<HoverInfo> HI;
-  SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
-      getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
+  auto CurLoc = sourceLocationInMainFile(SM, Pos);
+  if (!CurLoc) {
+    llvm::consumeError(CurLoc.takeError());
+    return llvm::None;
+  }
+  auto TokensAroundCursor =
+      syntax::spelledTokensTouching(*CurLoc, AST.getTokens());
+  if (TokensAroundCursor.empty())
+    return llvm::None;
+
+  // We prefer the identifiers if any, otherwise make use of the first token.
+  SourceLocation SourceLocationBeg = TokensAroundCursor.front().location();
+  for (const auto &Tok : TokensAroundCursor) {
+    if (Tok.kind() != tok::identifier)
+      continue;
+    SourceLocationBeg = Tok.location();
+    break;
+  }
 
+  llvm::Optional<HoverInfo> HI;
   if (auto Deduced = getDeducedType(AST.getASTContext(), SourceLocationBeg)) {
     HI = getHoverContents(*Deduced, AST.getASTContext(), Index);
   } else if (auto M = locateMacroAt(SourceLocationBeg, AST.getPreprocessor())) {
     HI = getHoverContents(*M, AST);
   } else {
-    auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
-    if (!Offset) {
-      llvm::consumeError(Offset.takeError());
-      return llvm::None;
-    }
+    auto Offset = SM.getFileOffset(*CurLoc);
     // Editors send the position on the left of the hovered character.
     // So our selection tree should be biased right. (Tested with VSCode).
     SelectionTree ST = SelectionTree::createRight(
-        AST.getASTContext(), AST.getTokens(), *Offset, *Offset);
+        AST.getASTContext(), AST.getTokens(), Offset, Offset);
     std::vector<const Decl *> Result;
     if (const SelectionTree::Node *N = ST.commonAncestor()) {
       auto Decls = explicitReferenceTargets(N->ASTNode, DeclRelation::Alias);
       if (!Decls.empty()) {
         HI = getHoverContents(Decls.front(), Index);
         // Look for a close enclosing expression to show the value of.
         if (!HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());

clang-tools-extra/clangd/SourceCode.h

llvm::Optional<Range> getTokenRange(const SourceManager &SM,
                                     const LangOptions &LangOpts,
                                     SourceLocation TokLoc);
 
 /// Return the file location, corresponding to \p P. Note that one should take
 /// care to avoid comparing the result with expansion locations.
 llvm::Expected<SourceLocation> sourceLocationInMainFile(const SourceManager &SM,
                                                         Position P);
 
-/// Get the beginning SourceLocation at a specified \p Pos in the main file.
-/// May be invalid if Pos is, or if there's no identifier or operators.
-/// The returned position is in the main file, callers may prefer to
-/// obtain the macro expansion location.
-SourceLocation getBeginningOfIdentifier(const Position &Pos,
-                                        const SourceManager &SM,
-                                        const LangOptions &LangOpts);
-
 /// Returns true iff \p Loc is inside the main file. This function handles
 /// file & macro locations. For macro locations, returns iff the macro is being
 /// expanded inside the main file.
 ///
 /// The function is usually used to check whether a declaration is inside the
 /// the main file.
 bool isInsideMainFile(SourceLocation Loc, const SourceManager &SM);
 

clang-tools-extra/clangd/SourceCode.cpp

llvm::Optional<Range> getTokenRange(const SourceManager &SM,
   if (!TokLoc.isValid())
     return llvm::None;
   SourceLocation End = Lexer::getLocForEndOfToken(TokLoc, 0, SM, LangOpts);
   if (!End.isValid())
     return llvm::None;
   return halfOpenToRange(SM, CharSourceRange::getCharRange(TokLoc, End));
 }
 
-namespace {
-
-enum TokenFlavor { Identifier, Operator, Whitespace, Other };
-
-bool isOverloadedOperator(const Token &Tok) {
-  switch (Tok.getKind()) {
-#define OVERLOADED_OPERATOR(Name, Spelling, Token, Unary, Binary, MemOnly)     \
-  case tok::Token:
-#define OVERLOADED_OPERATOR_MULTI(Name, Spelling, Unary, Binary, MemOnly)
-#include "clang/Basic/OperatorKinds.def"
-    return true;
-
-  default:
-    break;
-  }
-  return false;
-}
-
-TokenFlavor getTokenFlavor(SourceLocation Loc, const SourceManager &SM,
-                           const LangOptions &LangOpts) {
-  Token Tok;
-  Tok.setKind(tok::NUM_TOKENS);
-  if (Lexer::getRawToken(Loc, Tok, SM, LangOpts,
-                         /*IgnoreWhiteSpace*/ false))
-    return Other;
-
-  // getRawToken will return false without setting Tok when the token is
-  // whitespace, so if the flag is not set, we are sure this is a whitespace.
-  if (Tok.is(tok::TokenKind::NUM_TOKENS))
-    return Whitespace;
-  if (Tok.is(tok::TokenKind::raw_identifier))
-    return Identifier;
-  if (isOverloadedOperator(Tok))
-    return Operator;
-  return Other;
-}
-
-} // namespace
-
-SourceLocation getBeginningOfIdentifier(const Position &Pos,
-                                        const SourceManager &SM,
-                                        const LangOptions &LangOpts) {
-  FileID FID = SM.getMainFileID();
-  auto Offset = positionToOffset(SM.getBufferData(FID), Pos);
-  if (!Offset) {
-    log("getBeginningOfIdentifier: {0}", Offset.takeError());
-    return SourceLocation();
-  }
-
-  // GetBeginningOfToken(InputLoc) is almost what we want, but does the wrong
-  // thing if the cursor is at the end of the token (identifier or operator).
-  // The cases are:
-  //   1) at the beginning of the token
-  //   2) at the middle of the token
-  //   3) at the end of the token
-  //   4) anywhere outside the identifier or operator
-  // To distinguish all cases, we lex both at the
-  // GetBeginningOfToken(InputLoc-1) and GetBeginningOfToken(InputLoc), for
-  // cases 1 and 4, we just return the original location.
-  SourceLocation InputLoc = SM.getComposedLoc(FID, *Offset);
-  if (*Offset == 0) // Case 1 or 4.
-    return InputLoc;
-  SourceLocation Before = SM.getComposedLoc(FID, *Offset - 1);
-  SourceLocation BeforeTokBeginning =
-      Lexer::GetBeginningOfToken(Before, SM, LangOpts);
-  TokenFlavor BeforeKind = getTokenFlavor(BeforeTokBeginning, SM, LangOpts);
-
-  SourceLocation CurrentTokBeginning =
-      Lexer::GetBeginningOfToken(InputLoc, SM, LangOpts);
-  TokenFlavor CurrentKind = getTokenFlavor(CurrentTokBeginning, SM, LangOpts);
-
-  // At the middle of the token.
-  if (BeforeTokBeginning == CurrentTokBeginning) {
-    // For interesting token, we return the beginning of the token.
-    if (CurrentKind == Identifier || CurrentKind == Operator)
-      return CurrentTokBeginning;
-    // otherwise, we return the original loc.
-    return InputLoc;
-  }
-
-  // Whitespace is not interesting.
-  if (BeforeKind == Whitespace)
-    return CurrentTokBeginning;
-  if (CurrentKind == Whitespace)
-    return BeforeTokBeginning;
-
-  // The cursor is at the token boundary, e.g. "Before^Current", we prefer
-  // identifiers to other tokens.
-  if (CurrentKind == Identifier)
-    return CurrentTokBeginning;
-  if (BeforeKind == Identifier)
-    return BeforeTokBeginning;
-  // Then prefer overloaded operators to other tokens.
-  if (CurrentKind == Operator)
-    return CurrentTokBeginning;
-  if (BeforeKind == Operator)
-    return BeforeTokBeginning;
-
-  // Non-interesting case, we just return the original location.
-  return InputLoc;
-}
-
 bool isValidFileRange(const SourceManager &Mgr, SourceRange R) {
   if (!R.getBegin().isValid() || !R.getEnd().isValid())
     return false;
 
   FileID BeginFID;
   size_t BeginOffset = 0;
   std::tie(BeginFID, BeginOffset) = Mgr.getDecomposedLoc(R.getBegin());
 

clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp

for (const char *Test : Tests) {
     auto &PP = AST.getPreprocessor();
 
     // Known macros.
     for (int I = 1;; I++) {
       const auto ExpectedRefs = T.ranges(llvm::to_string(I));
       if (ExpectedRefs.empty())
         break;
 
-      auto Loc = getBeginningOfIdentifier(ExpectedRefs.begin()->start, SM,
-                                          AST.getLangOpts());
-      auto Macro = locateMacroAt(Loc, PP);
+      auto Loc = sourceLocationInMainFile(SM, ExpectedRefs.begin()->start);
+      ASSERT_TRUE(bool(Loc));
+      auto Macro = locateMacroAt(*Loc, PP);
       assert(Macro);
       auto SID = getSymbolID(Macro->Name, Macro->Info, SM);
 
       EXPECT_THAT(ExpectedRefs,
                   UnorderedElementsAreArray(ActualMacroRefs.MacroRefs[*SID]))
           << "Annotation=" << I << ", MacroName=" << Macro->Name
           << ", Test = " << Test;
     }

clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

)cpp");
   // Check all positions mentioned in the test return valid results.
   for (auto P : Source.points()) {
     size_t Offset = llvm::cantFail(positionToOffset(Source.code(), P));
     EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, P),
                          HasValue(StartOfFile.getLocWithOffset(Offset)));
   }
 }
 
-TEST(SourceCodeTests, GetBeginningOfIdentifier) {
-  std::string Preamble = R"cpp(
-struct Bar { int func(); };
-#define MACRO(X) void f() { X; }
-Bar* bar;
-  )cpp";
-  // First ^ is the expected beginning, last is the search position.
-  for (const std::string &Text : std::vector<std::string>{
-           "int ^f^oo();", // inside identifier
-           "int ^foo();",  // beginning of identifier
-           "int ^foo^();", // end of identifier
-           "int foo(^);",  // non-identifier
-           "^int foo();",  // beginning of file (can't back up)
-           "int ^f0^0();", // after a digit (lexing at N-1 is wrong)
-           "/^/ comments", // non-interesting token
-           "void f(int abc) { abc ^ ++; }",    // whitespace
-           "void f(int abc) { ^abc^++; }",     // range of identifier
-           "void f(int abc) { ++^abc^; }",     // range of identifier
-           "void f(int abc) { ++^abc; }",      // range of identifier
-           "void f(int abc) { ^+^+abc; }",     // range of operator
-           "void f(int abc) { ^abc^ ++; }",    // range of identifier
-           "void f(int abc) { abc ^++^; }",    // range of operator
-           "void f(int abc) { ^++^ abc; }",    // range of operator
-           "void f(int abc) { ++ ^abc^; }",    // range of identifier
-           "void f(int abc) { ^++^/**/abc; }", // range of operator
-           "void f(int abc) { ++/**/^abc; }",  // range of identifier
-           "void f(int abc) { ^abc^/**/++; }", // range of identifier
-           "void f(int abc) { abc/**/^++; }",  // range of operator
-           "void f() {^ }", // outside of identifier and operator
-           "int ^λλ^λ();",  // UTF-8 handled properly when backing up
-
-           // identifier in macro arg
-           "MACRO(bar->^func())",  // beginning of identifier
-           "MACRO(bar->^fun^c())", // inside identifier
-           "MACRO(bar->^func^())", // end of identifier
-           "MACRO(^bar->func())",  // begin identifier
-           "MACRO(^bar^->func())", // end identifier
-           "^MACRO(bar->func())",  // beginning of macro name
-           "^MAC^RO(bar->func())", // inside macro name
-           "^MACRO^(bar->func())", // end of macro name
-       }) {
-    std::string WithPreamble = Preamble + Text;
-    Annotations TestCase(WithPreamble);
-    auto AST = TestTU::withCode(TestCase.code()).build();
-    const auto &SourceMgr = AST.getSourceManager();
-    SourceLocation Actual = getBeginningOfIdentifier(
-        TestCase.points().back(), SourceMgr, AST.getLangOpts());
-    Position ActualPos = offsetToPosition(
-        TestCase.code(),
-        SourceMgr.getFileOffset(SourceMgr.getSpellingLoc(Actual)));
-    EXPECT_EQ(TestCase.points().front(), ActualPos) << Text;
-  }
-}
-
 TEST(SourceCodeTests, CollectIdentifiers) {
   auto Style = format::getLLVMStyle();
   auto IDs = collectIdentifiers(R"cpp(
   #include "a.h"
   void foo() { int xyz; int abc = xyz; return foo(); }
   )cpp",
                                 Style);
   EXPECT_EQ(IDs.size(), 7u);

 
 TEST(SourceCodeTests, GetMacros) {
   Annotations Code(R"cpp(
      #define MACRO 123
      int abc = MA^CRO;
    )cpp");
   TestTU TU = TestTU::withCode(Code.code());
   auto AST = TU.build();
-  auto Loc = getBeginningOfIdentifier(Code.point(), AST.getSourceManager(),
-                                      AST.getLangOpts());
-  auto Result = locateMacroAt(Loc, AST.getPreprocessor());
+  auto CurLoc = sourceLocationInMainFile(AST.getSourceManager(), Code.point());
+  ASSERT_TRUE(bool(CurLoc));
+  const auto *Id = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
+  ASSERT_TRUE(Id);
+  auto Result = locateMacroAt(Id->location(), AST.getPreprocessor());
   ASSERT_TRUE(Result);
   EXPECT_THAT(*Result, MacroName("MACRO"));
 }
 
 TEST(SourceCodeTests, IsInsideMainFile){
   TestTU TU;
   TU.HeaderCode = R"cpp(
     #define DEFINE_CLASS(X) class X {};