Page MenuHomePhabricator

[clangd] Color dependent names based on their heuristic target if they have one

Authored by nridge on Mar 26 2020, 5:20 PM.

Diff Detail

Event Timeline

nridge created this revision.Mar 26 2020, 5:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 5:20 PM

Adding some reviewers.

Great! Needs some tests though :-)


Can you move these into some logical order?
e.g. below the common cases, or paired with the non-dependent equivalents


Can you add tests for these changes?
This function is used for various things beyond semantic highlighting (such as rename) so it has its own tests.


nit: the quality is of the categorization (the "highlighting"), not the token.
And I don't think "heuristic" is a good name for DependentType etc - when DependentType conflicts with Field, it's *Field* that's heuristic!

I'd suggest something like enum HighlightPriority { Dependent, Resolved } so that < does the obvious thing.


nit: move the enum inside this function and return unsigned? No need to worry about names then, and represents that this is only used for prioritization.


why a limit of 2, vs a loop?


can we move this case into resolveConflict?


out of curiosity, *why* is the same token being highlighted as dependentname and resolved? Is it being traversed twice?

nridge retitled this revision from Color dependent names based on their heuristic target if they have one to [clangd] Color dependent names based on their heuristic target if they have one.Apr 9 2020, 12:43 PM
nridge updated this revision to Diff 256502.Apr 9 2020, 9:32 PM
nridge marked 7 inline comments as done.

Address review comments


I'm not aware of a scenario where we'd get more than two conflicting highlightings, except when macros are involved in which case we don't have a good way to disambiguate anyways.


It's for the reason you described in this comment: the resolved highlighting comes from findExplicitReferences(), and the dependent one form CollectExtraHighlightings.

sammccall accepted this revision.Apr 14 2020, 3:04 PM
sammccall added inline comments.

Oh yeah :-)
Can we add a comment somewhere about this case? e.g. on resolveConflict (this happens when...)

This revision is now accepted and ready to land.Apr 14 2020, 3:04 PM
nridge updated this revision to Diff 257596.Apr 14 2020, 9:56 PM

Address final review comment

This revision was automatically updated to reflect the committed changes.