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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

the implementation looks good, a few comments on the test.



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:61
+    }
+    if(isa<FunctionDecl>(D)) {
+      addToken(Loc, HighlightingKind::Function);
----------------
sammccall wrote:
> jvikstrom wrote:
> > 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.
> I don't think it needs a comment, especially if you're not actually highlighting them (because they have weird DeclarationNames)
> 
> > FIXMEs to the skip if statement in VisitNamedDecl
> I'm not actually sure there's anything to fix here - it's a bit hard to talk about constructor/destructor highlighting as distinct from type name highlighting in C++. If you want them highlighted as classes, then that should just start working when you start handling TypeLocs.
I think constructor/destructor can be categorized in the `function` group, like `entity.name.function.constructor`, `entity.name.function.destructor`


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:69
+    void $Function[[foo]]() {
+      int $Variable[[b]];
+      auto $Variable[[FN]] = [ $Variable[[b]]](int $Variable[[a]]) -> void {};
----------------
nit: even for the test code, could we make the code style consistent (like follow the LLVM code style) here?


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:76
+      R"cpp(
+    struct A {
+      A();
----------------
now we have 4 test cases, the purpose of each testcase is unclear to me (some of them are testing duplicated things), could we narrow the testcase so the each testcase just test one particular thing (e.g. one test for `Variable` and its references; one test for `Function` and its references;)?

I think the testcase here is to verify we don't highlighting the constructor/destructor, and operator, just 

```
R"cpp(
struct Foo {
  Foo();
  ~Foo();
  void $Function[[foo]]();
}
)cpp"
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64199