Page MenuHomePhabricator

[clangd] Added highlighting for variable references (declrefs)
ClosedPublic

Authored by jvikstrom on Jul 4 2019, 2:53 AM.

Event Timeline

jvikstrom created this revision.Jul 4 2019, 2:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2019, 2:53 AM
hokein added inline comments.Jul 4 2019, 5:02 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
52

The DeclRefExpr is a very general expression, which can reference a variable, a function, an enum, etc. I think we want to distinguish with them (rather than putting all into Variable type).

And we are missing large majority of entities now, I think we could start collecting more entities (class, method, enum, etc).

sammccall added inline comments.Jul 4 2019, 5:16 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
52

IMO the way to ensure consistency is to make the highlight a function of the decl, regardless of whether you found it in the decl itself, a declrefexpr, etc.

so something like:

// (overloaded)
addToken(SourceLocation Loc, NamedDecl *D) {
  if (isa<VarDecl>(D))
    return addToken(Loc, HighlightingKind::Variable);
  // ... etc
}

bool VisitDeclRefExpr(DeclRefExpr *Ref) { 
  // bail out for operators here (if it's only *usages* you don't want to highlight)
  addToken(Ref->getLocation(), Ref->getDecl());
}

bool VisitNamedDecl(NamedDecl *ND) {
  addToken(ND->getLocation(), ND);
}
jvikstrom updated this revision to Diff 208028.Jul 4 2019, 6:35 AM

Added overload for addToken and added more code to the test cases.

jvikstrom marked 2 inline comments as done.Jul 4 2019, 6:36 AM
jvikstrom updated this revision to Diff 208029.Jul 4 2019, 6:43 AM

Removed VisitVarDecl and VisitFuncDecl in favor of VisitNamedDecl.

jvikstrom updated this revision to Diff 208030.Jul 4 2019, 6:45 AM

Removed debug prints from test.

sammccall added inline comments.Jul 4 2019, 6:55 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
38

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.

65

note that methods, constructors, and destructors inherit from functiondecl, so if you want to exclude/distinguish those, order matters here

jvikstrom updated this revision to Diff 208041.Jul 4 2019, 7:43 AM
jvikstrom marked 2 inline comments as done.

Added testcae. Added another bailout from VisitNamedDecl.

jvikstrom added inline comments.Jul 4 2019, 7:44 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
38

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?

65

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.

sammccall added inline comments.Jul 4 2019, 7:59 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
38

Such a testcase would ensure you're not coloring any part of struct F { ~F(); } as a method, or operator << etc.

65

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.

sammccall accepted this revision.Jul 4 2019, 7:59 AM

(LG from my side)

This revision is now accepted and ready to land.Jul 4 2019, 7:59 AM
jvikstrom updated this revision to Diff 208123.Jul 5 2019, 12:32 AM

Added additional testcase.

hokein added a comment.Jul 5 2019, 1:34 AM

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

clang-tools-extra/clangd/SemanticHighlighting.cpp
65

I think constructor/destructor can be categorized in the function group, like entity.name.function.constructor, entity.name.function.destructor

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
69

nit: even for the test code, could we make the code style consistent (like follow the LLVM code style) here?

76

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"
jvikstrom updated this revision to Diff 208140.Jul 5 2019, 4:24 AM

Made tests more readable.

jvikstrom updated this revision to Diff 208143.Jul 5 2019, 4:29 AM

Separated into three testcases.

jvikstrom marked 4 inline comments as done.Jul 5 2019, 4:33 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
65

I'll have a look at constructors/destructors at the same time as I look at types

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
69

I think this should be consistent with LLVM code style.

hokein accepted this revision.Jul 5 2019, 5:07 AM

thanks, looks good.

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
62

nit: I'd add 2-space to the code, like:

R"cpp(
    struct AS {}
    ...
)cpp,
This revision was automatically updated to reflect the committed changes.
jvikstrom marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 6:06 AM