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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added inline comments.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:135
+  // Update the themeRuleMatcher that is used when highlighting. Also triggers a
+  // recolorization for all current highlighters. Safe to call multiple times.
+  public initialize(themeRuleMatcher: ThemeRuleMatcher) {
----------------
hokein wrote:
> nit: the comment is stale now.  I believe this function is only called whenever we reload a theme or at the first launch of the extension.
It's should only called when a theme is loaded (and we happen to load a theme when we initialize, but it's async).
Don't see why the comment is stale though, it seems to describe what it does to me.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:139
+      const options: vscode.DecorationRenderOptions = {
+        color : themeRuleMatcher.getBestThemeRule(scopes[0]).foreground,
+        // If the rangeBehavior is set to Open in any direction the
----------------
hokein wrote:
> just want to confirm: if we fail to find a matched theme rule, we return an empty decoration. I think vscode just doesn't do anything on an empty color?
vscode indeed does not do anything on empty colors.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:94
+    let line = [
+      {character : 1, length : 2, scopeIndex : 1},
+      {character : 5, length : 2, scopeIndex : 1},
----------------
hokein wrote:
> I believe the test code is correct, but the code here/below is complex and hard to follow. I think we need make the code more readable/understandable.
> 
> some suggestions:
> 
> - the line number is implicitly indicated by the index of the array, I think we can add one more field `line` to the structure (call HighlightingTokens).
> - and creates some helper functions (with proper names) that take the `HighlightingTokens` as parameter and generate the data you need e.g. `SemanticHighlightingLine[]`, `vscode.Range`.
> 
Started exporting the `SemanticHighlightingLine` interface and use that format in the tests to not create more interfaces (also means we don't need a helper for creating the SemanticHighlightingLine).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66219