sammccall added a comment. In D77811#2533237 <https://reviews.llvm.org/D77811#2533237>, @nridge wrote:
> Thanks a lot for working on this! I agree this is well aligned with the goals > of D77702 <https://reviews.llvm.org/D77702> (and in fact goes even further > and meets the goals of D66990 <https://reviews.llvm.org/D66990> via the > `declaration` modifier). Ooh, I hadn't seen D66990 <https://reviews.llvm.org/D66990>, thanks for the pointer and also giving the upstream feedback to LSP folks! I'm also really happy about the multi-dimensional nature of highlightings in the new protocol, looks like you pushed it in that directyon. >> - no split between primitive/typedef. (Is introducing a nonstandard kind is >> worth this distinction?) > > Here's my general take on this: > > - There is too much variety between programming languages to expect that a > standard list of highlighting kinds is going to satisfactorily cover all > languages. > - If LSP aims to be the basis for tools that are competitive with > purpose-built language-specific tools, it's reasonable to allow for clients > willing to go the extra mile in terms of customization (e.g. use a > language-specific theme which provides colors for language-specific token > kinds). > - Therefore, I would say yes, we should take the liberty of adding > nonstandard highlighting kinds and modifiers where it makes sense from a > language POV. Totally agree, I should have been more explicit. Introducing nonstandard kinds is **backwards-incompatible**. If the client doesn't understand primitiveType, then the token kind is now completely unknown. This could be a regression from the current state (type). But for primitive type specifically, the information we're losing is "auto denotes a type" which the client's non-semantic highlighting should manage anyway. So I agree we should do this. > That said... for typedef specifically, I wonder if it actually makes more > sense as a modifier than a kind. That is, have the kind be the target type > (falling back to Unknown/Dependent if we don't know), and have a modifier > flag for "the type is referred to via an alias" (as opposed to "the type is > referred to directly"). WDYT? Agree. Do you think it should be the *same* modifier as `deduced` which I included here? (In a similar vein, there's an argument for pointer, ref, rvalue-ref as modifiers) >> - no nonstandard local attribute This probably makes sense, I'm wondering if >> we want others and how they fit together. > > Are you referring to local-scope variables (i.e. `FunctionScope` in D95701 > <https://reviews.llvm.org/D95701>)? If so, I think the approach in that patch > is promising. Yes, exactly. ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:264 + $Class[[D]] $Field_decl[[E]]; + static double $StaticField_decl_static[[S]]; + static void $StaticMethod_decl_static[[bar]]() {} ---------------- nridge wrote: > Presumably, the highlighting kinds `StaticField` and `StaticMethod` are going > to be collapsed into `Field` and `Method` in a future change (after the > removal of TheiaSemanticHighlighting, I guess)? Yeah, merging any kinds that are exported with the same name should be NFC at that point. Hmm, though currently StaticField --> Variable, not Field (similarly StaticMethod --> Method). So we can have static fields be Variable+Static+ClassScope or Field+Static+ClassScope. I can see arguments for either... ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:718 + }; + )cpp", + }; ---------------- nridge wrote: > Should we add a test case for `_deprecated` as well? Oops, done! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77811/new/ https://reviews.llvm.org/D77811 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
