[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: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 &&
----------------
jvikstrom wrote:
> 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.
I thought that we are finding longest common prefix, but actually you are using a different one (I checked that "vscode-cpptools" is also using the same matching algorithm).

That sounds fair enough, could you add this context to the comment? 




================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:115
+  // The rules for the theme.
+  private rules: TokenColorRule[];
+  // A cache for the getBestThemeRule function.
----------------
I'd name it `themeRules`.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:133
+          scope.substr(0, rule.scope.length) === rule.scope &&
+          rule.scope.length > bestRule.scope.length)
+        // This rule matches and is more specific than the old rule.
----------------
I think we could simplify the code like 

```
if (scope.startsWith(rule.scope) && rule.scope.length > bestRule.scope.length) {
  ...
}
```


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:40
+    const rules = [
+      {scope : 'c.b', foreground : '1'},
+      {scope : 'a', foreground : '2'},
----------------
Even for the test, I'd use the real scopes, e.g. `variable.css`, `variable.other.less`.


================
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]);
----------------
jvikstrom wrote:
> 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?
`a` case is interesting here - we have duplicates, but actually we won't have duplicates in the theme rules (as we check the scope is being seen when parsing theme file).

I think for `a`, just use `assert.deepEqual(tm.getBestThemeRule('a'), rules[1]); // we match the first element if there are duplicates`; for others, use the scope.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65856