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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
