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 

struct Foo {
  void $Function[[foo]]();

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to