[PATCH] D64199: [clangd] Added highlighting for variable references (declrefs)

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:38
+  bool VisitNamedDecl(NamedDecl *ND) {
+    if (ND->getDeclName().isEmpty())
+      // Don't add symbols that don't have any length.
----------------
sammccall wrote:
> I think you might want to bail out (both here and in VisitDeclRefExpr) if the name kind isn't identifier.
> 
> Reason is you're only coloring the token at location, and most of the other name kinds can span multiple tokens or otherwise need special consideration.
I must have missed the Identifier NameKind because I was first-hand looking for something like that. 
Thanks.

Are you aware of any testcase I could add for this by the way?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:61
+    }
+    if(isa<FunctionDecl>(D)) {
+      addToken(Loc, HighlightingKind::Function);
----------------
sammccall wrote:
> note that methods, constructors, and destructors inherit from functiondecl, so if you want to exclude/distinguish those, order matters here
I'm aware of that, but thanks for the heads up. Although should I add it in a comment somewhere in the method? Also added an additional testcase for classes and FIXMEs to the skip if statement in VisitNamedDecl.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64199