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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
