[PATCH] D76896: Color dependent names based on their heuristic target if they have one

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Great! Needs some tests though :-)



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:651
+    void
+    VisitCXXDependentScopeMemberExpr(const CXXDependentScopeMemberExpr *E) {
+      Refs.push_back(
----------------
Can you move these into some logical order?
e.g. below the common cases, or paired with the non-dependent equivalents


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:651
+    void
+    VisitCXXDependentScopeMemberExpr(const CXXDependentScopeMemberExpr *E) {
+      Refs.push_back(
----------------
sammccall wrote:
> 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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:146
 
+enum class TokenQuality { Resolved, Heuristic };
+
----------------
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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:148
+
+TokenQuality evaluateTokenQuality(HighlightingKind Kind) {
+  return Kind == HighlightingKind::DependentType ||
----------------
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.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:157
+resolveConflict(ArrayRef<HighlightingToken> Tokens) {
+  if (Tokens.size() != 2)
+    return llvm::None;
----------------
why a limit of 2, vs a loop?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:209
       // should be in the highlightings.
       if (Conflicting.size() == 1)
         NonConflicting.push_back(TokRef.front());
----------------
can we move this case into resolveConflict?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:211
         NonConflicting.push_back(TokRef.front());
+      else if (auto Resolved = resolveConflict(Conflicting))
+        NonConflicting.push_back(*Resolved);
----------------
out of curiosity, *why* is the same token being highlighted as dependentname and resolved? Is it being traversed twice?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76896/new/

https://reviews.llvm.org/D76896