[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom marked an inline comment as done.
jvikstrom added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:177
       return;
+    if (TP->isPointerType() || TP->isLValueReferenceType())
+      // When highlighting dependant template types the type can be a pointer or
----------------
ilya-biryukov wrote:
> jvikstrom wrote:
> > ilya-biryukov wrote:
> > > jvikstrom wrote:
> > > > ilya-biryukov wrote:
> > > > > `RecursiveASTVisitor` also traverses the pointer and reference types, why does it not reach the inner `TemplateTypeParmType` in the cases you describe?
> > > > The D in `using D = ...` `typedef ... D` does not have a TypeLoc (at least not one that is visited). Therefore we use the VisitTypedefNameDecl (line 121) to get the location of `D` to be able to highlight it. And we just send the typeLocs typeptr to addType (which is a Pointer for `using D = T*;`)...
> > > > 
> > > > But maybe we should get the underlying type before we call addType with TypePtr? Just a while loop on line 123 basically (can we have multiple PointerTypes nested in each other actually?)
> > > > 
> > > > Even if we keep it in addType the comment is actually wrong, because it obviously works when for the actual "type occurrences" for `D` (so will fix that no matter what). This recursion will just make us add more duplicate tokens...
> > > Could we investigate why `RecursiveASTVisitor` does not visit the `TypeLoc` of a corresponding decl?
> > > Here's the code from `RecursiveASTVisitor.h` that should do the trick:
> > > ```
> > > DEF_TRAVERSE_DECL(TypeAliasDecl, {
> > >   TRY_TO(TraverseTypeLoc(D->getTypeSourceInfo()->getTypeLoc()));
> > >   // We shouldn't traverse D->getTypeForDecl(); it's a result of
> > >   // declaring the type alias, not something that was written in the
> > >   // source.
> > > })
> > > ```
> > > 
> > > If it doesn't, we are probably holding it wrong.
> > There just doesn't seem to be a TypeLoc for the typedef'ed Decl.  We can get the `T*` TypeLoc (with `D->getTypeSourceInfo()->getTypeLoc()`). But there isn't one for `D`. Even the `D->getTypeForDecl` returns null.
> > 
> > And I have no idea where I'd even start debugging that. Or if it's even a bug
> > 
> I may have misinterpreted the patch. Are we trying to add highlightings for the names of using aliases here? E.g. for the following range:
> ```
> template <class T>
> struct Foo {
>   using [[D]] = T**;
> };
> ```
> 
> Why isn't this handled in `VisitNamedDecl`?
> We don't seem to call this function for `TypedefNameDecl` at all and it actually weird. Is this because we attempt to highlight typedefs as their underlying types?
So currently using aliases and typedefs are highlighted the same as the underlying type (in most cases). One case where they aren't is when the underlying type is a template parameter (which is what this patch is trying to solve).


> Why isn't this handled in VisitNamedDecl?

The Decl is actually visited in `VisitNamedDecl`, however as it is a `TypeAliasDecl` which we do not have a check for in the addToken function it will not get highlighted in that visit.

Actually, could add a check for `TypeAliasDecl` in `addToken` (should probably be a check for `TypedefNameDecl` to cover both `using ...` and `typedef ...`) and move the code from the `VisitTypedefNameDecl` to the `addToken` function inside that check instead.



> We don't seem to call this function for TypedefNameDecl at all and it actually weird. Is this because we attempt to highlight typedefs as their underlying types?


Don't understand what you mean. What function? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66516