sammccall accepted this revision.
sammccall added a comment.
Thanks! Sorry for letting this drop.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:79
+ if (auto *Var = dyn_cast<VarDecl>(Decl))
+ return !isa<ParmVarDecl>(Var) && Var->isThisDeclarationADefinition();
+ return isa<ObjCCategoryDecl>(Decl) || isa<ObjCImplDecl>(Decl);
----------------
ckandeler wrote:
> I'm not 100% sure about this one, by the way. I've just never heard anyone
> talk about a "parameter definition".
ParmVarDecls should be marked as definition. They definitely are in the formal
C++ sense, and they're so similar to local variables that I think we have to be
consistent.
(There are other formally-definitions that we're not marking here like
NamespaceDecl, but they seem unlikely to cause confusion).
> I've just never heard anyone talk about a "parameter definition".
Yeah, I suspect this is mostly because:
- there's no definition/declaration distinction - you can't forward-declare a
param.
- imprecise mental models of params as aliases to the written args rather than
copies of them (at least I sometimes think this way)
But I think it's important we treat these consistently with e.g. local
variables since they're so similar.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:62
+ if (T.Modifiers & (1 << I)) {
+ if (I != uint32_t(HighlightingModifier::Declaration) || !hasDef)
+ OS << '_' << static_cast<HighlightingModifier>(I);
----------------
can you add a brief comment here: "// _decl_def is common and redundant, just
print _def instead."
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127403/new/
https://reviews.llvm.org/D127403
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
