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