[PATCH] D65856: [clangd] Added class for mapping TokenColorRules to their associated clangd TextMate scope

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:65
+    const name =
+        vscode.workspace.getConfiguration('workbench').get('colorTheme');
+    if (typeof name != 'string') {
----------------
maybe just `get<string>`, and get rid of the following check.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:119
 
+export class ThemeRules {
+  // The rules for the theme.
----------------
I'd name it  `ThemeRuleMatcher`?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:131
+    this.rules.forEach((rule) => {
+      if (rule.scope.length <= scope.length &&
+          scope.substr(0, rule.scope.length) === rule.scope &&
----------------
hmm, here comes the question, we need algorithm to find the best match rule. not doing it in this patch is fine, please add a FIXME.

could you also document what's the strategy using here?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:143
 // Get all token color rules provided by the theme.
-function loadTheme(themeName: string): Promise<TokenColorRule[]> {
+async function loadTheme(themeName: string): Promise<ThemeRules> {
   const extension =
----------------
nit: no need to change this method, you could construct the `ThemeRules` from the returned results.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65856