[PATCH] D64257: [clangd] Added highlighting for non-builtin types

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:68
+
+    TagDecl *D = TL.getTypePtr()->getAsTagDecl();
+    if (!D)
----------------
We are only interested in `TagDecl`, maybe use the `VisitTagLoc` callback, so that you can get rid of the filtering code above.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:72
+
+    if (auto RD = dyn_cast<CXXRecordDecl>(D)) {
+      if (auto DC = RD->getDestructor()) {
----------------
nit: auto => `const auto *`, we usually spell out the `pointer type` explicitly.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:73
+    if (auto RD = dyn_cast<CXXRecordDecl>(D)) {
+      if (auto DC = RD->getDestructor()) {
+        auto Range = DC->getSourceRange();
----------------
Here is the case:

```
class Foo {
   ~Foo
 // ^~~ we get a TypeLoc whose TagDecl is a cxxRecordDecl.
}
```
not sure this is expected in clang AST, but it is unreasonable in highlighting context -- ideally, we want to highlight `~Foo` as a destructor (we may encounter a tricky case, like `~ /*comment*/ Foo()`, but I assume this is rarce, should be fine), @sammccall, @ilya-biryukov, thoughts?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:99
     }
+    if(isa<CXXRecordDecl>(D)) {
+      addToken(Loc, HighlightingKind::Type);
----------------
nit: clang-format


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:214
+  case HighlightingKind::Type:
+    return "entity.name.type.cpp";
   case HighlightingKind::NumKinds:
----------------
`entity.name.type.class.cpp`?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:29
   Function,
+  Type,
 
----------------
`Type` is general, can match different types, I think we want distinguish different types (class, enum, etc).

Since this patch is highlighting `class`, I think the name should be `Class`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64257