[PATCH] D65856: [clangd] Added class for mapping TokenColorRules to their associated clangd TextMate scope
Johan Vikström via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 13 08:25:31 PDT 2019
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 &&
> 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):
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);
+ assert.deepEqual(tm.getBestThemeRule('a.b'), rules);
> 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);` as is or be consistent?
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits