sammccall added inline comments.
================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:73 // This check is for not getting two entries when there are anonymous // structs. It also makes us not highlight namespace qualifiers. For // elaborated types the actual type is highlighted as an inner TypeLoc. ---------------- jvikstrom wrote: > hokein wrote: > > this comment is stale with this patch, now we are highlighting namespace, > > it seems more natural to do it here, we can get a NestedNameSpecifierLoc > > from an `ElaboratedTypeLoc ` (so that we don't need the > > `TraverseNestedNameSpecifierLoc`). > Doing it in VisitTypeLoc means that we fail on this testcase: > > ``` > namespace $Namespace[[aa]] { > namespace $Namespace[[bb]] { > struct $Class[[A]] {}; > } > } > $Namespace[[aa]]::$Namespace[[bb]]::$Class[[A]] $Variable[[a]]; > ``` > > It can't detect the `bb` namespace qualifier in `aa::bb::A a;`. > > Maybe there is some way of solving this without > `TraverseNestedNameSpecifierLoc` that I am not aware of? > Also don't know how I'd get a `NestedNameSpecifierLoc` from an > `ElaboratedTypeLoc`. TraverseNestedNameSpecifierLoc is the right way to deal with namespaces-as-prefixes, I think it's best to follow that consistently. (For namespaces not-as-prefixes, you've got namespacealiasdecl, namespacedecl, using-directives... I think that's everything) ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:87 + bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNSLoc) { + if (NestedNameSpecifier *NNS = NNSLoc.getNestedNameSpecifier()) + if (NNS->getKind() == NestedNameSpecifier::Namespace || ---------------- if you're just doing something and then calling base, can you make this Visit instead of Traverse? ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:131 + // The target namespace can not be found in any other way. + addToken(NAD->getTargetNameLoc(), HighlightingKind::Namespace); + return; ---------------- this doesn't fit with the contract of this addToken() function, which should only highlight the provided token. Instead, can you call this from VisitNamespaceAliasDecl? ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:86 template<typename T> - struct $Class[[C]] : abc::A<T> { + struct $Class[[C]] : $Namespace[[abc]]::A<T> { typename T::A* D; ---------------- can you add a case where we spell with leading colons e.g. `::abc::A` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64492/new/ https://reviews.llvm.org/D64492 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits