hokein added inline comments.
================
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:131
+ this.rules.forEach((rule) => {
+ if (rule.scope.length <= scope.length &&
+ scope.substr(0, rule.scope.length) === rule.scope &&
----------------
jvikstrom wrote:
> hokein wrote:
> > hmm, here comes the question, we need algorithm to find the best match
> > rule. not doing it in this patch is fine, please add a FIXME.
> >
> > could you also document what's the strategy using here?
> But isn't the best match for a scope just the rule that is the longest prefix
> of the scope (or a perfect match if one exists)? (as that should be the most
> specific rule)
that depends, given the fact that the theme scope maybe not exactly the same
with the scope provided by clangd. The longest-prefix match may not work well
on cases like `a.b.<theme-specific-field>.c.d` and `a.b.c.d`. But we don't
know yet without further experiment, I'd add a FIXME for revisiting the
strategy here.
================
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:51
+ // The rules for the current theme.
+ themeRules: ThemeRuleMatcher;
fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
----------------
the variable name should be updated as well.
================
Comment at:
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:119
+ constructor(rules: TokenColorRule[]) { this.rules = rules; }
+ // Returns the best rule for a scope. The best rule for a scope is the rule
+ // that is the longest prefix of the scope (unless a perfect match exists in
----------------
`The best rule for a scope is the rule ...` sounds like implementation details,
should be moved to the function body.
================
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
================
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]);
----------------
I'd use `assert.deepEqual(tm.getBestThemeRule('c.b').scope, 'c.b');` to make
the code more readable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65856/new/
https://reviews.llvm.org/D65856
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
