nridge 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:
> > > 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.
> > > we chose to highlight typedefs same as their underlying type (e.g. if
> > > it's a class we highlight the typedef as class too)
> >
> > So this version should do that. For the typedef part.
> >
> > When there are references with typedefs
> > `SomeTypedefTypeThatIsNotBuiltinAndHasNoTagDecl R = ...` this will not
> > highlight the `SomeTypedefTypeThatIsNotBuiltinAndHasNoTagDecl` because
> > RecursiveASTVisitor does not traverse into typedeftypelocs, so we'd need to
> > add the getUnderlyingType thing in VisitTypeLoc as well.
> > Also need to do this for auto.
> >
> > And, does it make sense to highlight function typedefs as their return type?
> >
> >
> > But I'm a bit worried this current approach might not be really sensible.
> > The 'short circuit' when we find a TagDecl is because otherwise we get
> > weird results at times. (for example TemplateInstantiations, if
> > instantiated with a builtin we'd highlight that as a Primitive while it
> > should be a class because of traversal order)
> >
> > I guess it would be possible to create a TypeVisitor kind of like how you
> > get deduced types from a typeptr, but that would be a lot of boilerplate.
> >
> > Maybe what we could do instead of stopping the traversal early when finding
> > a TagDecl is add one field for each Leaf type in UnderlyingTypeVisitor.
> > When returning we'd then return the Record if there is one or Enum if there
> > is one, ... , or a Builtin if none of the others exist.
> >
> > Is there maybe some other way of finding the "correct" type for a type
> > hierarchy or maybe doing the very basic ranking would be enough? (Record >
> > Enum > TemplateTypeParm > ObjC stuff > Builtin)
> >
> > And, does it make sense to highlight function typedefs as their return type?
> I also think it's confusing. But we need to fallback to something if we see a
> function type (or some other unhandled type) and I believe we
> currently just don't highlight those typedefs at all, which seems bad.
>
> I would just have a separate highlighting kind for typedefs (regardless of
> what the underlying type is).
> This would allow to avoid both technical problems (simplifying the
> implementation) and logical questions (e.g. 'should we highlight typedefs to
> functions as their return type?)
> I would just have a separate highlighting kind for typedefs (regardless of
> what the underlying type is).
FWIW, this is what Eclipse CDT does.
(Although clangd's current choice to highlight typedefs based on their target
type is certainly an interesting one. From a user's point of view I can see
arguments in either direction. On the one hand, revealing information about the
target type is useful. On the other hand, knowing that a type //is// an alias
for something else can also be useful.)
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
