ilya-biryukov 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
----------------
jvikstrom wrote:
> 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?
> So currently using aliases and typedefs are highlighted the same as the
> underlying type (in most cases).
Thanks for clarifying this. This is where my confusion is coming from.
A few question to try understanding the approach taken (sorry if that's too
detailed, I am probably missing the context here)
- What do we fallback to? From my reading of the code, we do not highlight them
at all if the underlying type is not one of the predefined cases.
- Why are pointers and **l-value** references special? What about arrays,
r-value references, function types, pack expansions, etc.?
> Don't understand what you mean. What function?
We don't call `VisitNamedDecl` from `VisitTypedefNameDecl`. I guess that's
intentional if we try to highlight them as underlying types.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66516/new/
https://reviews.llvm.org/D66516
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
