sammccall added a comment.
Some context for me: I did also start some work on supporting modifiers, and
after some experiments with VSCode concluded it was too early (no support in
any shipped themes, I saw some hard-to-explain interactions with the default
syntax highlighting...).
My plan was to revisit once this was in the standard and VSCode had some
out-of-the-box support. If there's a goal to implement already in other editors
(Theia?) that's a good reason to move forward sooner, but we should be wary of
getting too far ahead of VSCode I think, as they're the ones who will mostly
decide how the standard mostly gets interpreted.
The unfinished code is in D77811 <https://reviews.llvm.org/D77811> for what
it's worth (no tests, not sure if the scope is right etc). With that out of the
way...
---
A couple of observations about the spec:
1: LSP modifiers are clearly designed to be orthogonal to the token kind (in
principle at least, even if `static namespace` isn't a thing).
Some of the most useful modifiers mentioned in the spec (e.g. readonly and
definition) are certainly going to apply to a wide range of kinds. And I
imagine these will often be orthogonal in presentation too (e.g. underline all
definitions, color indicates what it is).
I think this makes a lot of sense and would like our design to reflect this
internally (e.g. modifiers as boolean flags in HighlightingToken>) rather than
having modifiers+kind encode some internal enum. While it's OK to have some
modifiers that aren't orthogonal internally, I worry about *starting* there.
And I'm skeptical that some of those added here are best implemented this way.
2: The "predefined" modifiers/kinds in the semanticToken spec are a bit of a
dilemma.
We care about lots of clients, and know from experience that a) we have very
little influence on how vscode interprets the spec and b) we have some
influence on other clients, but they mostly copy vscode by default
Because servers, client plugins, editors, and themes typically come from
different vendors, getting modifiers/kinds highlighted well out-of-the-box is
going to be really hard unless there's alignment on what the modifiers/kinds
*are*.
I think the predefined values in the spec are probably the best/most likely way
to get this alignment, so there's a lot of value in sticking to them closely.
**On the other hand** we'll never capture all interesting C++ semantics in a
language-neutral spec, and there are some seriously questionable choices
("member" as a kind rather than an attribute or at least method/field?).
There's no "subclasses" at least for kinds, so we have to make a call one way
or the other. I do think we should stick to the standard names where we
possibly can, and fight (and probably lose) to get `member` fixed, `local`
added etc in the spec.
---
I'm not sure that making the results of the new API easy to exactly map back
onto the results of the old API is something we should be taking as a hard
design goal, which I know makes transition a bit painful :-(
Similar to the first point, we don't want our design constrained too much by an
interface that will go away.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:450
+ None = 0,
+ Local = 1,
+ Static = 2,
----------------
I like this a lot. There's a function `computeScope` in Quality.cpp that
determines if something's class/function/file scoped, though for function-local
I guess `Decl::isLexicallyWithinFunctionOrMethod()` is enough.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:451
+ Local = 1,
+ Static = 2,
+ Member = 4,
----------------
I think we should probably call this StaticMember (internally), to avoid
accidentally conflating the various meanings of `static`.
(Which reminds me, I'd love to make hover on `static` explain what it means
here!)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77702/new/
https://reviews.llvm.org/D77702
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
