[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 added inline comments.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:8
+// The information clangd sends when highlightings should be updated.
+interface SemanticHighlightingParams {
+  // The information about the text document where these highlightings should be
----------------
sorry for not being clear in the previous comment, I think we should mirror definitions from the LSP [proposal](https://github.com/microsoft/vscode-languageserver-node/pull/367/files#diff-ac2186ae4b8ba5cc9f17deebc42c9e28R13), the same to the `SemanticHighlightingInformation`.

You may need to add a `vscode-languageserver-types` dependency.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:32
+
+export const NotificationType = new vscodelc.NotificationType<{}, void>(
+    'textDocument/semanticHighlighting');
----------------
I think here we should use `SemanticHighlightingParams` in the generic type parameter list


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:38
+export class SemanticHighlightingFeature implements vscodelc.StaticFeature {
+  scopeLookupTable: string[][];
+  fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
----------------
could you add a comment on this?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:63
+  handleNotification(params: SemanticHighlightingParams) {
+    const tokenLines =
+        params.lines.map((line): SemanticHighlightingInformation => {
----------------
this seems doesn't do anything, can we defer it to a follow-up patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65998/new/

https://reviews.llvm.org/D65998