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

[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: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 &&
----------------
hokein wrote:
> 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
If the scope's length is less than the rule's length the rule can not be a prefix.
(Not sure I fully follow with what you mean in the first sentence though)


If we check common prefixes we run into this weird case (this is taken from the Light+ theme):
```
variable.css
variable.scss
variable.other.less
variable
```
With that kind of matching we have now this means that `variable.other.less` will match `variable.other` and `variable.other.less` and the variables would be colored as less variables while they should be `variable.other`. Same goes for `variable.other.field`.

And even if `variable.other.less` did not exist `variable.other` would still match `variable.css` and now be highlighted as css variables.


================
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]);
----------------
hokein wrote:
> I'd use `assert.deepEqual(tm.getBestThemeRule('c.b').scope, 'c.b');` to make the code more readable.
> 
For the `a` case we are interested in the foreground color as well. Should I change the others and keep `assert.deepEqual(tm.getBestThemeRule('a'), rules[1]);` as is or be consistent?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65856