[PATCH] D64492: [clangd] Added highlightings for namespace specifiers.

[Sam McCall via Phabricator via cfe-commits]

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