[PATCH] D75447: [clangd] Make use of token buffers in semantic highlighting

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3302af83ef79: [clangd] Make use of token buffers in semantic 
highlighting (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75447

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -128,40 +129,39 @@
   return Result;
 }
 
+// For a macro usage `DUMP(foo)`, we want:
+//  - DUMP --> "macro"
+//  - foo --> "variable".
+SourceLocation getHighlightableSpellingToken(SourceLocation L,
+ const SourceManager ) {
+  if (L.isFileID())
+return SM.isWrittenInMainFile(L) ? L : SourceLocation{};
+  // Tokens expanded from the macro body contribute no highlightings.
+  if (!SM.isMacroArgExpansion(L))
+return {};
+  // Tokens expanded from macro args are potentially highlightable.
+  return getHighlightableSpellingToken(SM.getImmediateSpellingLoc(L), SM);
+}
+
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const SourceManager ,
-   const LangOptions )
-  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+  HighlightingsBuilder(const ParsedAST )
+  : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
+LangOpts(AST.getLangOpts()) {}
 
   void addToken(HighlightingToken T) { Tokens.push_back(T); }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
+Loc = getHighlightableSpellingToken(Loc, SourceMgr);
 if (Loc.isInvalid())
   return;
-if (Loc.isMacroID()) {
-  // Only intereseted in highlighting arguments in macros (DEF_X(arg)).
-  if (!SourceMgr.isMacroArgExpansion(Loc))
-return;
-  Loc = SourceMgr.getSpellingLoc(Loc);
-}
-
-// Non top level decls that are included from a header are not filtered by
-// topLevelDecls. (example: method declarations being included from
-// another file for a class from another file).
-// There are also cases with macros where the spelling loc will not be in
-// the main file and the highlighting would be incorrect.
-if (!isInsideMainFile(Loc, SourceMgr))
-  return;
+const auto *Tok = TB.spelledTokenAt(Loc);
+assert(Tok);
 
-auto Range = getTokenRange(SourceMgr, LangOpts, Loc);
-if (!Range) {
-  // R should always have a value, if it doesn't something is very wrong.
-  elog("Tried to add semantic token with an invalid range");
-  return;
-}
-Tokens.push_back(HighlightingToken{Kind, *Range});
+auto Range = halfOpenToRange(SourceMgr,
+ Tok->range(SourceMgr).toCharRange(SourceMgr));
+Tokens.push_back(HighlightingToken{Kind, std::move(Range)});
   }
 
   std::vector collect(ParsedAST ) && {
@@ -211,6 +211,7 @@
   }
 
 private:
+  const syntax::TokenBuffer 
   const SourceManager 
   const LangOptions 
   std::vector Tokens;
@@ -311,7 +312,7 @@
 std::vector getSemanticHighlightings(ParsedAST ) {
   auto  = AST.getASTContext();
   // Add highlightings for AST nodes.
-  HighlightingsBuilder Builder(AST.getSourceManager(), C.getLangOpts());
+  HighlightingsBuilder Builder(AST);
   // Highlight 'decltype' and 'auto' as their underlying types.
   CollectExtraHighlightings(Builder).TraverseAST(C);
   // Highlight all decls and references coming from the AST.


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -128,40 +129,39 @@
   return Result;
 }
 
+// For a macro usage `DUMP(foo)`, we want:
+//  - DUMP --> "macro"
+//  - foo --> "variable".
+SourceLocation getHighlightableSpellingToken(SourceLocation L,
+ const SourceManager ) {
+  if (L.isFileID())
+return SM.isWrittenInMainFile(L) ? L : SourceLocation{};
+  // Tokens expanded from the macro body contribute no highlightings.
+  if (!SM.isMacroArgExpansion(L))
+return {};
+  // Tokens expanded from macro args are 

[PATCH] D75447: [clangd] Make use of token buffers in semantic highlighting

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 247805.
kadircet added a comment.

- Use spelledTokenAt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75447

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -128,40 +129,39 @@
   return Result;
 }
 
+// For a macro usage `DUMP(foo)`, we want:
+//  - DUMP --> "macro"
+//  - foo --> "variable".
+SourceLocation getHighlightableSpellingToken(SourceLocation L,
+ const SourceManager ) {
+  if (L.isFileID())
+return SM.isWrittenInMainFile(L) ? L : SourceLocation{};
+  // Tokens expanded from the macro body contribute no highlightings.
+  if (!SM.isMacroArgExpansion(L))
+return {};
+  // Tokens expanded from macro args are potentially highlightable.
+  return getHighlightableSpellingToken(SM.getImmediateSpellingLoc(L), SM);
+}
+
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const SourceManager ,
-   const LangOptions )
-  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+  HighlightingsBuilder(const ParsedAST )
+  : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
+LangOpts(AST.getLangOpts()) {}
 
   void addToken(HighlightingToken T) { Tokens.push_back(T); }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
+Loc = getHighlightableSpellingToken(Loc, SourceMgr);
 if (Loc.isInvalid())
   return;
-if (Loc.isMacroID()) {
-  // Only intereseted in highlighting arguments in macros (DEF_X(arg)).
-  if (!SourceMgr.isMacroArgExpansion(Loc))
-return;
-  Loc = SourceMgr.getSpellingLoc(Loc);
-}
-
-// Non top level decls that are included from a header are not filtered by
-// topLevelDecls. (example: method declarations being included from
-// another file for a class from another file).
-// There are also cases with macros where the spelling loc will not be in
-// the main file and the highlighting would be incorrect.
-if (!isInsideMainFile(Loc, SourceMgr))
-  return;
+const auto *Tok = TB.spelledTokenAt(Loc);
+assert(Tok);
 
-auto Range = getTokenRange(SourceMgr, LangOpts, Loc);
-if (!Range) {
-  // R should always have a value, if it doesn't something is very wrong.
-  elog("Tried to add semantic token with an invalid range");
-  return;
-}
-Tokens.push_back(HighlightingToken{Kind, *Range});
+auto Range = halfOpenToRange(SourceMgr,
+ Tok->range(SourceMgr).toCharRange(SourceMgr));
+Tokens.push_back(HighlightingToken{Kind, std::move(Range)});
   }
 
   std::vector collect(ParsedAST ) && {
@@ -211,6 +211,7 @@
   }
 
 private:
+  const syntax::TokenBuffer 
   const SourceManager 
   const LangOptions 
   std::vector Tokens;
@@ -311,7 +312,7 @@
 std::vector getSemanticHighlightings(ParsedAST ) {
   auto  = AST.getASTContext();
   // Add highlightings for AST nodes.
-  HighlightingsBuilder Builder(AST.getSourceManager(), C.getLangOpts());
+  HighlightingsBuilder Builder(AST);
   // Highlight 'decltype' and 'auto' as their underlying types.
   CollectExtraHighlightings(Builder).TraverseAST(C);
   // Highlight all decls and references coming from the AST.


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -128,40 +129,39 @@
   return Result;
 }
 
+// For a macro usage `DUMP(foo)`, we want:
+//  - DUMP --> "macro"
+//  - foo --> "variable".
+SourceLocation getHighlightableSpellingToken(SourceLocation L,
+ const SourceManager ) {
+  if (L.isFileID())
+return SM.isWrittenInMainFile(L) ? L : SourceLocation{};
+  // Tokens expanded from the macro body contribute no highlightings.
+  if (!SM.isMacroArgExpansion(L))
+return {};
+  // Tokens expanded from macro args are potentially highlightable.
+  return 

[PATCH] D75447: [clangd] Make use of token buffers in semantic highlighting

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:162
+[&](const syntax::Token ) { return Tok.location() < Loc; });
+assert(Tok);
+

`&& Tok->location() == Loc`, right?

(I think this would make a nice method 
`TokenBuffer::spelledTokenAt(SourceLocation) -> const Token*`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75447



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75447: [clangd] Make use of token buffers in semantic highlighting

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 247695.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75447

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -128,40 +129,41 @@
   return Result;
 }
 
+// For a macro usage `DUMP(foo)`, we want:
+//  - DUMP --> "macro"
+//  - foo --> "variable".
+SourceLocation getHighlightableSpellingToken(SourceLocation L,
+ const SourceManager ) {
+  if (L.isFileID())
+return SM.isWrittenInMainFile(L) ? L : SourceLocation{};
+  // Tokens expanded from the macro body contribute no highlightings.
+  if (!SM.isMacroArgExpansion(L))
+return {};
+  // Tokens expanded from macro args are potentially highlightable.
+  return getHighlightableSpellingToken(SM.getImmediateSpellingLoc(L), SM);
+}
+
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const SourceManager ,
-   const LangOptions )
-  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+  HighlightingsBuilder(const ParsedAST )
+  : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
+LangOpts(AST.getLangOpts()) {}
 
   void addToken(HighlightingToken T) { Tokens.push_back(T); }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
+Loc = getHighlightableSpellingToken(Loc, SourceMgr);
 if (Loc.isInvalid())
   return;
-if (Loc.isMacroID()) {
-  // Only intereseted in highlighting arguments in macros (DEF_X(arg)).
-  if (!SourceMgr.isMacroArgExpansion(Loc))
-return;
-  Loc = SourceMgr.getSpellingLoc(Loc);
-}
-
-// Non top level decls that are included from a header are not filtered by
-// topLevelDecls. (example: method declarations being included from
-// another file for a class from another file).
-// There are also cases with macros where the spelling loc will not be in
-// the main file and the highlighting would be incorrect.
-if (!isInsideMainFile(Loc, SourceMgr))
-  return;
-
-auto Range = getTokenRange(SourceMgr, LangOpts, Loc);
-if (!Range) {
-  // R should always have a value, if it doesn't something is very wrong.
-  elog("Tried to add semantic token with an invalid range");
-  return;
-}
-Tokens.push_back(HighlightingToken{Kind, *Range});
+const auto *Tok = llvm::partition_point(
+TB.spelledTokens(SourceMgr.getMainFileID()),
+[&](const syntax::Token ) { return Tok.location() < Loc; });
+assert(Tok);
+
+auto Range = halfOpenToRange(SourceMgr,
+ Tok->range(SourceMgr).toCharRange(SourceMgr));
+Tokens.push_back(HighlightingToken{Kind, std::move(Range)});
   }
 
   std::vector collect(ParsedAST ) && {
@@ -211,6 +213,7 @@
   }
 
 private:
+  const syntax::TokenBuffer 
   const SourceManager 
   const LangOptions 
   std::vector Tokens;
@@ -311,7 +314,7 @@
 std::vector getSemanticHighlightings(ParsedAST ) {
   auto  = AST.getASTContext();
   // Add highlightings for AST nodes.
-  HighlightingsBuilder Builder(AST.getSourceManager(), C.getLangOpts());
+  HighlightingsBuilder Builder(AST);
   // Highlight 'decltype' and 'auto' as their underlying types.
   CollectExtraHighlightings(Builder).TraverseAST(C);
   // Highlight all decls and references coming from the AST.


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -128,40 +129,41 @@
   return Result;
 }
 
+// For a macro usage `DUMP(foo)`, we want:
+//  - DUMP --> "macro"
+//  - foo --> "variable".
+SourceLocation getHighlightableSpellingToken(SourceLocation L,
+ const SourceManager ) {
+  if (L.isFileID())
+return SM.isWrittenInMainFile(L) ? L : SourceLocation{};
+  // Tokens expanded from the macro body contribute no highlightings.
+  if 

[PATCH] D75447: [clangd] Make use of token buffers in semantic highlighting

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:134
 public:
-  HighlightingsBuilder(const SourceManager ,
-   const LangOptions )
-  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+  HighlightingsBuilder(ParsedAST )
+  : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),

nit: this can be const



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:148
+// though.
+if (!SourceMgr.isMacroArgExpansion(Loc))
   return;

only if this is a macro!



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:150
   return;
-if (Loc.isMacroID()) {
-  // Only intereseted in highlighting arguments in macros (DEF_X(arg)).
-  if (!SourceMgr.isMacroArgExpansion(Loc))
-return;
-  Loc = SourceMgr.getSpellingLoc(Loc);
-}
-
-// Non top level decls that are included from a header are not filtered by
-// topLevelDecls. (example: method declarations being included from
-// another file for a class from another file).
-// There are also cases with macros where the spelling loc will not be in
-// the main file and the highlighting would be incorrect.
-if (!isInsideMainFile(Loc, SourceMgr))
+auto Toks = TB.spelledForExpanded(TB.expandedTokens(Loc));
+if (!Toks || Toks->empty() ||

I think using TokenBuffer to translate expanded -> spelled isn't actually an 
improvement here as the API exposes several possibilities that can't actually 
happen in our case.

The specialized translation from expanded to spelled location is *policy*, and 
it's pretty easy to implement I think:

```
// For a macro usage `DUMP(foo)`, we want:
//  - DUMP --> "macro"
//  - foo --> "variable".
SourceLocation getHighlightableSpellingToken(SourceLocation L) {
  if (L.isFileID()) return SM.isWrittenInMainFile(L) ? L : {};
  // Tokens expanded from the macro body contribute no highlightings.
  if (!SM.isMacroArgExpansion(L)) return {};
  // Tokens expanded from macro args are potentially highlightable.
  return getHighlightableSpellingToken(SM.getImmediateSpellingLocation(L));
}
```

once you have the spelling location, getting the range from tokenbuffer is easy 
:-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75447



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75447: [clangd] Make use of token buffers in semantic highlighting

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 247612.
kadircet added a comment.

- Add forgetten macroid check, before looking for an macroargexpansion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75447

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -131,37 +132,30 @@
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const SourceManager ,
-   const LangOptions )
-  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+  HighlightingsBuilder(ParsedAST )
+  : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
+LangOpts(AST.getLangOpts()) {}
 
   void addToken(HighlightingToken T) { Tokens.push_back(T); }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
-if (Loc.isInvalid())
+// This enables highlighting tokens coming from macro expansions, e.g.:
+// #define MACRO SOME_NAME
+// int MACRO;
+// ~ -> this will get highlighted as a macro token.
+// As `de-conflicting` logic inside `collect` would drop these otherwise.
+// There were tests specifically checking for that, not sure if it is 
needed
+// though.
+if (Loc.isMacroID() && !SourceMgr.isMacroArgExpansion(Loc))
   return;
-if (Loc.isMacroID()) {
-  // Only intereseted in highlighting arguments in macros (DEF_X(arg)).
-  if (!SourceMgr.isMacroArgExpansion(Loc))
-return;
-  Loc = SourceMgr.getSpellingLoc(Loc);
-}
-
-// Non top level decls that are included from a header are not filtered by
-// topLevelDecls. (example: method declarations being included from
-// another file for a class from another file).
-// There are also cases with macros where the spelling loc will not be in
-// the main file and the highlighting would be incorrect.
-if (!isInsideMainFile(Loc, SourceMgr))
+auto Toks = TB.spelledForExpanded(TB.expandedTokens(Loc));
+if (!Toks || Toks->empty() ||
+!isInsideMainFile(Toks->front().location(), SourceMgr))
   return;
 
-auto Range = getTokenRange(SourceMgr, LangOpts, Loc);
-if (!Range) {
-  // R should always have a value, if it doesn't something is very wrong.
-  elog("Tried to add semantic token with an invalid range");
-  return;
-}
-Tokens.push_back(HighlightingToken{Kind, *Range});
+auto Range = halfOpenToRange(
+SourceMgr, Toks->front().range(SourceMgr).toCharRange(SourceMgr));
+Tokens.push_back(HighlightingToken{Kind, std::move(Range)});
   }
 
   std::vector collect(ParsedAST ) && {
@@ -211,6 +205,7 @@
   }
 
 private:
+  const syntax::TokenBuffer 
   const SourceManager 
   const LangOptions 
   std::vector Tokens;
@@ -311,7 +306,7 @@
 std::vector getSemanticHighlightings(ParsedAST ) {
   auto  = AST.getASTContext();
   // Add highlightings for AST nodes.
-  HighlightingsBuilder Builder(AST.getSourceManager(), C.getLangOpts());
+  HighlightingsBuilder Builder(AST);
   // Highlight 'decltype' and 'auto' as their underlying types.
   CollectExtraHighlightings(Builder).TraverseAST(C);
   // Highlight all decls and references coming from the AST.


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -131,37 +132,30 @@
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const SourceManager ,
-   const LangOptions )
-  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+  HighlightingsBuilder(ParsedAST )
+  : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
+LangOpts(AST.getLangOpts()) {}
 
   void addToken(HighlightingToken T) { Tokens.push_back(T); }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
-if (Loc.isInvalid())
+// This enables highlighting tokens coming from macro expansions, e.g.:
+// #define MACRO SOME_NAME
+// int MACRO;
+//   

[PATCH] D75447: [clangd] Make use of token buffers in semantic highlighting

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added reviewers: hokein, sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.
kadircet added a parent revision: D75446: [clang][Syntax] Handle macro 
arguments in spelledForExpanded.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75447

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -130,37 +131,30 @@
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const SourceManager ,
-   const LangOptions )
-  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+  HighlightingsBuilder(ParsedAST )
+  : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
+LangOpts(AST.getLangOpts()) {}
 
   void addToken(HighlightingToken T) { Tokens.push_back(T); }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
-if (Loc.isInvalid())
+// This enables highlighting tokens coming from macro expansions, e.g.:
+// #define MACRO SOME_NAME
+// int MACRO;
+// ~ -> this will get highlighted as a macro token.
+// As `de-conflicting` logic inside `collect` would drop these otherwise.
+// There were tests specifically checking for that, not sure if it is 
needed
+// though.
+if (!SourceMgr.isMacroArgExpansion(Loc))
   return;
-if (Loc.isMacroID()) {
-  // Only intereseted in highlighting arguments in macros (DEF_X(arg)).
-  if (!SourceMgr.isMacroArgExpansion(Loc))
-return;
-  Loc = SourceMgr.getSpellingLoc(Loc);
-}
-
-// Non top level decls that are included from a header are not filtered by
-// topLevelDecls. (example: method declarations being included from
-// another file for a class from another file).
-// There are also cases with macros where the spelling loc will not be in
-// the main file and the highlighting would be incorrect.
-if (!isInsideMainFile(Loc, SourceMgr))
+auto Toks = TB.spelledForExpanded(TB.expandedTokens(Loc));
+if (!Toks || Toks->empty() ||
+!isInsideMainFile(Toks->front().location(), SourceMgr))
   return;
 
-auto Range = getTokenRange(SourceMgr, LangOpts, Loc);
-if (!Range) {
-  // R should always have a value, if it doesn't something is very wrong.
-  elog("Tried to add semantic token with an invalid range");
-  return;
-}
-Tokens.push_back(HighlightingToken{Kind, *Range});
+auto Range = halfOpenToRange(
+SourceMgr, Toks->front().range(SourceMgr).toCharRange(SourceMgr));
+Tokens.push_back(HighlightingToken{Kind, std::move(Range)});
   }
 
   std::vector collect(ParsedAST ) && {
@@ -210,6 +204,7 @@
   }
 
 private:
+  const syntax::TokenBuffer 
   const SourceManager 
   const LangOptions 
   std::vector Tokens;
@@ -341,7 +336,7 @@
 std::vector getSemanticHighlightings(ParsedAST ) {
   auto  = AST.getASTContext();
   // Add highlightings for AST nodes.
-  HighlightingsBuilder Builder(AST.getSourceManager(), C.getLangOpts());
+  HighlightingsBuilder Builder(AST);
   // Highlight 'decltype' and 'auto' as their underlying types.
   CollectExtraHighlightings(Builder).TraverseAST(C);
   // Highlight all decls and references coming from the AST.


Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -130,37 +131,30 @@
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const SourceManager ,
-   const LangOptions )
-  : SourceMgr(SourceMgr), LangOpts(LangOpts) {}
+  HighlightingsBuilder(ParsedAST )
+  : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
+LangOpts(AST.getLangOpts()) {}
 
   void addToken(HighlightingToken T) { Tokens.push_back(T); }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
-if (Loc.isInvalid())
+// This enables highlighting tokens coming from macro