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