Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The heuristic of guessing a concrete type seems fragile, and may not work for all cases. Instead of trying out test to guess the underlying type, I think we could introduce a new highlighting kind (e.g. entity.name.type.dependent) for this.
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
199 | This could be member functions, a case is like template<class T> class Foo { public: void foo() { this->foo(); } }; |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
199 | Thanks for the example. Do you have a suggestion for how to discriminate this case? To me, it would seem logical to do it based on syntax (highlight as a member function if the expression forms the function name of a function call expression). That would require navigating from the expression to its parent node. Is there a way to do that? |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
199 | There is no way to do this in C++. It's much better to pick a separate highlighting kind for dependent names, this follows the actual semantics of C++. |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
176 | I'm doubting we will encounter other exceptions. struct X { template<class T> static T t; }; template<typename T> void f() { X::t<T>; // this is a `UnresolvedLookupExpr`. } | |
199 | +1, I think we should just highlight them as a dependent type. | |
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp | ||
54 | thanks for catching this! |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
199 | Of course, any attempt to disambiguate between a member function and a field would be heuristic only. I figured that would be better than nothing. But if you prefer using a separate highlighting for dependent names that resolve to a function or a variable, we could do that. (Hokein, I assume you don't actually mean using the dependent *type* highlighting. Using a type highlighting for something we know is not a type, but rather a function or variable, would be rather confusing.) |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
199 |
I'd suggest we'd better follow this. The heuristic could fail in some cases. For these cases, the wrong highlighting'd confuse users too.
sorry for the confusion, I meant for anything that could not resolve to a specific declaration (e.g. CXXDependentScopeMemberExpr, UnresolvedLookupExpr), we use the new dependent highlighting kind. |
I do feel strongly that types and non-types should be highlighted differently, so the updated patch adds two new dependent highlightings, one for types and one for variables/functions.
I see your point here, no objection.
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
174 | nit: remove the {} and elsewhere, LLVM prefers not adding {} if the body only contains a single statement. | |
181 | I think we should use E->getName() since we are highlighting the NameLoc below. | |
229 | nit: we have kindForType for hanlding all types, so I'd move the logic of detecting the dependent type there. |
Address review comments
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
229 | 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. |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
229 | 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. |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
519 | Maybe have a separate category for all dependent entities instead, i.e. use This would allow to specify a single highlighting for both names by stopping at dependent subcategory. |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
519 | 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. |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
229 | 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? | |
519 | +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. |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
229 | 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. |
Is this what you had in mind?
I'm not seeing where the kindForType part comes in. In particular, it seems like it would be silly to call kindForType in WalkUpFromDependentNameTypeLoc(), because we know the answer will be HighlightingKind::DependentType.
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
175 | Could we highlighting based on the kinds of E->decls()? If they're all the same, you could just use the corresponding highlighting kind. If there are no decls() or some of them have different highlighting kinds (e.g. static and non-static member functions), you could fallback to DependentName. | |
180 | Same here. Could we try guessing the highlighting type based on E->decls()? |
Updated to use heuristics based on OverloadExpr::decl()
Also folded VisitUnresolvedLookupExpr and VisitUnresolvedMemberExpr together
into a single VisitOverloadExpr
I like how we went from using heuristics, to not using heuristics, to using heuristics again :)
But admittedly, the new heuristics are more accurate because they're based on phase 1 lookup results rather than just syntax, so I'm happy with the outcome!
I also liked the irony of it :-)
Mostly LG, just NITs from my side. Happy to LGTM when they're addressed.
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
109 | Could we do this in kindForDecl instead? | |
112 | Maybe simplify the rest of the loop body to the following code? auto Kind = ...; if (!Kind || Result && Result != Kind) return llvm::None; Result = Kind; If you want to have fewer assignments, we could also do: if (!Result) Result = Kind; at the end. But I'd just keep it a tad simpler instead. |
LGTM from my side, but not marking as accepted yet to make sure @hokein has a chance to take a final look.
nit: remove the enclosing {}.