hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:219
+ bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+ addToken(L.getNameLoc(), HighlightingKind::DependentType);
+ return true;
----------------
nridge wrote:
> ilya-biryukov wrote:
> > nridge wrote:
> > > hokein wrote:
> > > > nit: we have `kindForType` for hanlding all types, so I'd move the
> > > > logic of detecting the dependent type there.
> > > I did try this, but it doesn't quite work, because `VisitTypeLoc` adds
> > > the highlighting to the `TypeLoc`'s `getBeginLoc()`. For something like
> > > `typename T::type`, that highlights the `typename` token rather than the
> > > `type` token. By contrast, here I add the highlighting to the
> > > `DependentNameTypeLoc`'s `getNameLoc()` which will correctly highlight
> > > the `type` token.
> > You'd want to implement `WalkUpFromDependentNameTypeLoc` instead of
> > `Visit*` to avoid adding extra highlightings in `VisitTypeLoc`.
> >
> > In fact, I'm surprised we're not seeing them now.
> We're not seeing extra highlightings with the current patch, because
> `VisitTypeLoc` does not add any highlightings for dependent types
> (`kindForType()` returns `None` for them).
>
> So, I don't think there's a problem with using `VisitDependentNameTypeLoc`?
I'm second on Ilya's suggestion. It follows the existing pattern (see
WalkupFrom* methods above) and make the code clearer, and we can move the
`DependentType` to `kindForType`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67901/new/
https://reviews.llvm.org/D67901
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
