This is an archive of the discontinued LLVM Phabricator instance.

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

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 :-)

clang-tools-extra/clangd/FindTarget.cpp
659

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

659

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.

clang-tools-extra/clangd/SemanticHighlighting.cpp
146

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.

148

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.

157

why a limit of 2, vs a loop?

215–216

can we move this case into resolveConflict?

217

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

clang-tools-extra/clangd/SemanticHighlighting.cpp
157

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.

217

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.
clang-tools-extra/clangd/SemanticHighlighting.cpp
217

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.
nridge added inline comments.Mar 16 2022, 1:19 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
157

I should've listened to you when you suggested a loop :)

I overlooked the case where findExplicitReferences() contributes more than one candidate token because a dependent name is heuristically resolved to an overload set, in which case we get one target for each overload.

Fixed in D121775.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 16 2022, 1:19 AM
sammccall added inline comments.Mar 16 2022, 8:10 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
157

I think this is a good sign, my tendency is to borrow too many problems from the future anyway :-)