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