[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, looks good, just a few nits.



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:34
+    return true;
+  if (!Name.isIdentifier())
+    return false;
----------------
nit: I think the check is redundant, getAsIdentifierInfo() will return nullptr if `!Name.isIdentifier()`.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:37
+  auto *II = Name.getAsIdentifierInfo();
+  return II && II->getName() != "";
+}
----------------
nit: `!II->getName().empty()`


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:169
 private:
-  void addTokenForTypedef(SourceLocation Loc, const TypedefNameDecl *TD) {
-    auto *TSI = TD->getTypeSourceInfo();
-    if (!TSI)
-      return;
-    // Try to highlight as underlying type.
-    if (addType(Loc, TSI->getType().getTypePtrOrNull()))
-      return;
-    // Fallback to the typedef highlighting kind.
-    addToken(Loc, HighlightingKind::Typedef);
-  }
-
-  bool addType(SourceLocation Loc, const Type *TP) {
+  llvm::Optional<HighlightingKind> kindForType(const Type *TP) {
     if (!TP)
----------------
how about moving out this method (and `kindForDecl`) of the class, they seem to not depend on any fields of the class?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:212
+      return HighlightingKind::Parameter;
+    if (const VarDecl *VD = dyn_cast<VarDecl>(D))
+      return VD->isStaticDataMember()
----------------
nit: auto


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:260
+  void addToken(SourceLocation Loc, const NamedDecl *D) {
+    auto K = kindForDecl(D);
+    if (!K)
----------------
maybe 

```
if (auto K = kindForDecl(D))
   addToken(Loc, *K);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67341