hokein added inline comments.

Comment at: 
+    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: 
+  // The rules for the current theme.
+  themeRules: ThemeRuleMatcher;
   fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
the variable name should be updated as well.

Comment at: 
+  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: 
+      // 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: 
+    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.

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to