[PATCH] D64257: [clangd] Added highlighting for class and enum types

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:38
   bool VisitNamedDecl(NamedDecl *ND) {
-    // FIXME: (De)Constructors/operator need to be highlighted some other way.
+    // Constructors have a TypePtr TagDecl that is a Function, therefore we need
+    // to get constructors as a NamedDecl instead.
----------------
jvikstrom wrote:
> hokein wrote:
> > I don't understand this comment -- when visiting the constructor AST, we get a TagTypeLoc, and its underlying Decl is a `CXXConstructorDecl`
> So the Constructor TypeLoc does not have a TagTypeDecl and is not a TagTypeLoc. When we get the TypePtr of the constructor it's a "FunctionProtoType" and there is no way to distinguish it from other functions. Therefore we need to get the constructor decls as NamedDecls..
> 
> The comment was written badly though. This version should be better now I hope.
ah, I see, that make senses, thanks for the explanation.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:52
 
     if (ND->getDeclName().isEmpty())
       // Don't add symbols that don't have any length.
----------------
if you move this to `addToken` (and change function parameter type to `NamedDecl`), then you don't need the check on Line 79.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:73
+    // elaborated types the actual type is highlighted as an inner TypeLoc.
+    TL.getTypePtr()->dump();
+    if (TL.getTypeLocClass() == TypeLoc::TypeLocClass::Elaborated)
----------------
nit: remove the debug dump.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:77
+
+    if (TL.getTypePtr())
+      if (TagDecl *TD = TL.getTypePtr()->getAsTagDecl())
----------------
Again, you can save one cost of `TL.getTypePtr()`.

```
  if (const auto* TypePtr = TL.getTypePtr())
    if (const auto* TD = TypePtr->getAsTagDecl())
```


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:87
   void addToken(SourceLocation Loc, const Decl *D) {
+    // Destructors have a TypeLoc where the underlying TypePtr is a RecordDecl.
+    if (isa<RecordDecl>(D)) {
----------------
how about?

```
We highlight class decls, constructor decls and destructor decls as `Class` type. The destructor decls are handled in `VisitTypeLoc` (we will visit a TypeLoc where the underlying Type is a CXXRecordDecl).
``` 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64257