[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:131
+    this.rules.forEach((rule) => {
+      if (rule.scope.length <= scope.length &&
+          scope.substr(0, rule.scope.length) === rule.scope &&
----------------
jvikstrom wrote:
> hokein wrote:
> > 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?
> But isn't the best match for a scope just the rule that is the longest prefix of the scope (or a perfect match if one exists)? (as that should be the most specific rule)
that depends, given the fact that the theme scope maybe not exactly the same with the scope provided by clangd. The longest-prefix match may not work well on cases like `a.b.<theme-specific-field>.c.d` and `a.b.c.d`.  But we don't know yet without further experiment,  I'd add a FIXME for revisiting the strategy here.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:51
+  // The rules for the current theme.
+  themeRules: ThemeRuleMatcher;
   fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
----------------
the variable name should be updated as well.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:119
+  constructor(rules: TokenColorRule[]) { this.rules = rules; }
+  // Returns the best rule for a scope. The best rule for a scope is the rule
+  // that is the longest prefix of the scope (unless a perfect match exists in
----------------
`The best rule for a scope is the rule ...` sounds like implementation details, should be moved to the function body.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:128
+      // Find the rule wich is the longest prefix of scope.
+      if (rule.scope.length <= scope.length &&
+          scope.substr(0, rule.scope.length) === rule.scope &&
----------------
The algorithm doesn't seems correct to me, if scope.length > rule.scope.length, then we drop it.

I think we should 
- calculate the common prefix between two scopes
- update the bestRule if the length of common prefix is greater than the current best length


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:47
+    const tm = new SM.ThemeRuleMatcher(rules);
+    assert.deepEqual(tm.getBestThemeRule('c.b'), rules[0]);
+    assert.deepEqual(tm.getBestThemeRule('a.b'), rules[2]);
----------------
I'd use `assert.deepEqual(tm.getBestThemeRule('c.b').scope, 'c.b');` to make the code more readable.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65856