[PATCH] D66219: [clangd] Added a colorizer to the vscode extension.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:144
+// sends.
+export class Colorizer<T> {
+  private files: Map<string, Map<number, SemanticHighlightingLine>> = new Map();
----------------
The generic `T` doesnt seem to be used?

Regarding the name, the word `colorization` is from vscode cpp-tools extension, I think in clangd we should keep using `Hightlight` for consistency, maybe use `Highlighter`?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:145
+export class Colorizer<T> {
+  private files: Map<string, Map<number, SemanticHighlightingLine>> = new Map();
+  private colorizers: Map<string, IFileColorizer> = new Map();
----------------
could you add some documentation for these fields?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:146
+  private files: Map<string, Map<number, SemanticHighlightingLine>> = new Map();
+  private colorizers: Map<string, IFileColorizer> = new Map();
+  private themeRuleMatcher: ThemeRuleMatcher;
----------------
It seems that, all files are sharing with duplicated implementations in this patch, storing duplications is wasteful.

Maybe we could make the `colorize` method as a virtual method -- in the unittest, we create a subclass of `Colorizer` and override the `colorize`  method.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:164
+  public setFileLines(uriString: string, tokens: SemanticHighlightingLine[]) {
+    // Patch in the new highlightings to the highlightings cache. If this is the
+    // first time the file should be highlighted a new colorizer and a file
----------------
we are patching the incremental diff to get a "global" highlighting for the whole file, but it maybe not correct -- clangd doesn't emit diff outside of the max line number of the file, if we delete a few lines at the end of the file, then we will keep the removed lines in the map.

I think this is a current limitation, could you check with the current behavior of vscode if we pass a out-of-range to the decoration API? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66219