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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
