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
----------------
ilya-biryukov wrote:
> 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.
Summarizing the offline discussion:
- we chose to highlight typedefs same as their underlying type (e.g. if it's a
class we highlight the typedef as class too)
- we need to ensure all typdefs are highlighted in **some** color. That means
we need to handle all composite types in this function and recurse into those.
That involves handling at least functions, arrays, all reference types (not
just l-value references).
- we should add tests for this.
@jvikstrom, please let me know if I missed something.
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
