hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
mostly good with a few nits.
================
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:9
+// Parameters for the semantic highlighting (server-side) push notification.
+interface SemanticHighlightingParams {
+ // The text document that has to be decorated with the semantic highlighting
----------------
could you add a comment describing this is a mirror of LSP definition?
================
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:16
+}
+
+// Contains the highlighting information for a specified line.
----------------
nit: I'd remove this blank line to make `SemanticHighlightingParams` and
`SemanticHighlightingInformation` group closer as they are mirrors of LSP
definitions.
================
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:27
+
+// A single SemanticHighlightingToken that clangd sent.
+interface SemanticHighlightingToken {
----------------
the comment seems not precise to me, this is the data decoded from the
base64-encoded string sent by clangd.
================
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:33
+ length: number;
+ // The TextMate scope index to the clangd scopes.
+ scope: number;
----------------
nit: scope lookup table.
================
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:34
+ // The TextMate scope index to the clangd scopes.
+ scope: number;
+}
----------------
nit: maybe name it `scopeIndex`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65998/new/
https://reviews.llvm.org/D65998
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
