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

[Sam McCall via Phabricator via cfe-commits]

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