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

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 13 06:47:22 PDT 2019

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list