nridge marked 2 inline comments as done.
nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:219
+ bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+ addToken(L.getNameLoc(), HighlightingKind::DependentType);
+ return true;
----------------
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`?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:505
+ case HighlightingKind::DependentType:
+ return "entity.name.type.dependent.cpp";
+ case HighlightingKind::DependentName:
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Maybe have a separate category for all dependent entities instead, i.e. use
> > `entity.name.dependent.type.cpp` and `entity.name.dependent.other.cpp`?
> >
> > This would allow to specify a single highlighting for both names by
> > stopping at `dependent` subcategory.
> > I'm not sure how this plays into the default colors provided in the editors
> > that support semantic highlighting...
> Having a dedicate dependent entity doesn't align with the existing textmate
> patterns (the `dependent` type should be under the `entity.name.type`
> umbrella) -- most highlighters don't have a specific pattern for
> `dependent`, so we'll fallback to the `entity.name.type` color. If we use
> `entity.name.dependent.type.cpp`, we'll fall back to `entity.name` color.
+1, I think it's more important that dependent types are highlighted as types
out-of-the-box in cases where the theme contains a highlighting for types but
not one specifically for dependent types.
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
