[PATCH] D65998: [clangd] Added the vscode SemanticHighlighting feature code but did not enable it in the client.

[Haojian Wu via Phabricator via cfe-commits]

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