[PATCH] D127403: [clangd] Implement semantic token modifier "definition"

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This is fantastic, I'm really not sure how I missed it, sorry :-(



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:857
             Tok.addModifier(HighlightingModifier::Deprecated);
           // Do not treat an UnresolvedUsingValueDecl as a declaration.
           // It's more common to think of it as a reference to the
----------------
nit: move this comment to above the inner if?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:863
+              Tok.addModifier(HighlightingModifier::Declaration);
+            if (const auto Func = dyn_cast<FunctionDecl>(Decl)) {
+              if (Func->isThisDeclarationADefinition())
----------------
ckandeler wrote:
> dgoldman wrote:
> > Instead of doing it like this, I wonder if would make more sense to call getDefinition from https://cs.github.com/llvm/llvm-project/blob/ae071a59bc6cc09e0e0043e0ef1d9725853f1681/clang-tools-extra/clangd/XRefs.cpp#L76 and if it matches Decl, add the Definition modifier?
> Won't that incur an additional look-up or some other type of work? I'm not deeply familiar with the implementation, but a cursory glance seems to suggests that isThisDeclarationADefinition() is just an accessor, while getDefinition() "does things".
Yeah, getDefinition may need to walk over all the redecl chain to find the actual def, which we don't care about.
Probably not a big deal, but it's not quite the right abstraction, and that function isn't exposed publicly currently anyway.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:863
+              Tok.addModifier(HighlightingModifier::Declaration);
+            if (const auto Func = dyn_cast<FunctionDecl>(Decl)) {
+              if (Func->isThisDeclarationADefinition())
----------------
sammccall wrote:
> ckandeler wrote:
> > dgoldman wrote:
> > > Instead of doing it like this, I wonder if would make more sense to call getDefinition from https://cs.github.com/llvm/llvm-project/blob/ae071a59bc6cc09e0e0043e0ef1d9725853f1681/clang-tools-extra/clangd/XRefs.cpp#L76 and if it matches Decl, add the Definition modifier?
> > Won't that incur an additional look-up or some other type of work? I'm not deeply familiar with the implementation, but a cursory glance seems to suggests that isThisDeclarationADefinition() is just an accessor, while getDefinition() "does things".
> Yeah, getDefinition may need to walk over all the redecl chain to find the actual def, which we don't care about.
> Probably not a big deal, but it's not quite the right abstraction, and that function isn't exposed publicly currently anyway.
can you pull these checks out into a function `bool isUniqueDefinition` or so?

("unique" because I think we're intentionally returning false for things like NamespaceDecls which are definitions but may be multiply-defined)


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:54
     OS << '$' << T.Kind;
     for (unsigned I = 0;
          I <= static_cast<uint32_t>(HighlightingModifier::LastModifier); ++I) {
----------------
I wonder if we want to special case when we're printing `_def` to not print `_decl` and instead assert that it is present?

It's a bit irregular but as you've seen this gets everywhere in the tests, and the noise level seems concerning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127403