This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Added class for mapping TokenColorRules to their associated clangd TextMate scope
ClosedPublic

Authored by jvikstrom on Aug 7 2019, 3:17 AM.

Event Timeline

jvikstrom created this revision.Aug 7 2019, 3:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 3:17 AM

Rebased into master.

jvikstrom updated this revision to Diff 214778.Aug 13 2019, 1:06 AM

Updated to use string[][] as scopes.

jvikstrom updated this revision to Diff 214779.Aug 13 2019, 1:14 AM

Updated patch description.

I think we could make the layering clearer:

  • we now have a list of theme color rules, and scope names provided by clangd; and we we want to find the best match theme rule for a particular clangd scope;
  • we could define a function like getBestThemeRule(clangd_scope_name: string) - when we render a token, we get the index of the lookup table, find the exact clangd scope name, and call the function to get the corresponding theme rule;
  • we need to cache the result to avoid re-computing, we could make the function as a class method;
jvikstrom updated this revision to Diff 214813.Aug 13 2019, 5:36 AM

Lazy load the best theme rule for a scope.

jvikstrom updated this revision to Diff 214814.Aug 13 2019, 5:37 AM

Added a missing test case.

hokein added inline comments.Aug 13 2019, 6:46 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
7

maybe just get<string>, and get rid of the following check.

15

I'd name it ThemeRuleMatcher?

27

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?

49

nit: no need to change this method, you could construct the ThemeRules from the returned results.

jvikstrom updated this revision to Diff 214833.Aug 13 2019, 7:35 AM
jvikstrom marked 4 inline comments as done.

Address comments.

jvikstrom updated this revision to Diff 214834.Aug 13 2019, 7:36 AM

Removed stray edits from loadTheme.

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
27

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)

hokein added inline comments.Aug 13 2019, 8:09 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
6

the variable name should be updated as well.

21

The best rule for a scope is the rule ... sounds like implementation details, should be moved to the function body.

27

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.

30

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
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
24

I'd use assert.deepEqual(tm.getBestThemeRule('c.b').scope, 'c.b'); to make the code more readable.

jvikstrom updated this revision to Diff 214844.Aug 13 2019, 8:24 AM
jvikstrom marked 4 inline comments as done.

Changed variable name.

jvikstrom added inline comments.Aug 13 2019, 8:25 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
30

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.

clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
24

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?

jvikstrom updated this revision to Diff 214845.Aug 13 2019, 8:33 AM
jvikstrom marked an inline comment as done.

Added fixme for ranking.

hokein added inline comments.Aug 14 2019, 2:13 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
17

I'd name it themeRules.

30

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?

35

I think we could simplify the code like

if (scope.startsWith(rule.scope) && rule.scope.length > bestRule.scope.length) {
  ...
}
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
17

Even for the test, I'd use the real scopes, e.g. variable.css, variable.other.less.

24

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.

jvikstrom updated this revision to Diff 215065.Aug 14 2019, 2:53 AM
jvikstrom marked 4 inline comments as done.

Simplified matching code. Use real scopes for test.

hokein accepted this revision.Aug 14 2019, 5:07 AM

thanks, looks good.

This revision is now accepted and ready to land.Aug 14 2019, 5:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 5:12 AM