[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D67901#1687308 <https://reviews.llvm.org/D67901#1687308>, @nridge wrote:

> 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.



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:156
+  bool VisitUnresolvedLookupExpr(UnresolvedLookupExpr *E) {
+    if (canHighlightName(E->getName())) {
+      addToken(E->getNameLoc(), HighlightingKind::DependentName);
----------------
nit: remove the `{}` and elsewhere, LLVM prefers not adding `{}` if the body only contains a single statement.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:163
+  bool VisitUnresolvedMemberExpr(UnresolvedMemberExpr *E) {
+    if (canHighlightName(E->getMemberName())) {
+      addToken(E->getNameLoc(), HighlightingKind::DependentName);
----------------
I think we should use `E->getName()` since we are highlighting the `NameLoc` below.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:219
+  bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+    addToken(L.getNameLoc(), HighlightingKind::DependentType);
+    return true;
----------------
nit: we have `kindForType` for hanlding all types, so I'd move the logic of detecting the dependent type there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67901/new/

https://reviews.llvm.org/D67901