[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:81
   void addToken(SourceLocation Loc, const Decl *D) {
+    if (isa<CXXConstructorDecl>(D)) {
+      addToken(Loc, HighlightingKind::Class);
----------------
jvikstrom wrote:
> hokein wrote:
> > nit: move this around `if (isa<RecordDecl>(D)) {` since they are related to `Class`, and we should have a comment describing the highlighting behavior of `class, constructor, and destructor`.
> I don't really know what you mean with this comment after the move RecordDecl around (or rather where to put the comment and what to put in it)
> I wrote a comment but don't know if that's  really helpful
> I don't really know what you mean with this comment after the move RecordDecl around (or rather where to put the comment and what to put in it)

sorry for the confusion, I meant we group "class" cases together just as what your code does now (there are going to be many of kinds, it would be clear if we break them into group and sort them).

> I wrote a comment but don't know if that's really helpful

The comment should comment something **not obvious but important** in the code, so here we don't have a `if (isa<CXXDestructorDecl>(D)))` case to handle the destructor, but the code does highlight destructors  -- for destructors, we get a TagTypeLoc whose underlying decl is CXXRecordDecl in VisitTypeLoc), I think it deserves a comment here.


================
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.
----------------
I don't understand this comment -- when visiting the constructor AST, we get a TagTypeLoc, and its underlying Decl is a `CXXConstructorDecl`


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:73
+
+    if (!TL.getTypePtr())
+      return true;
----------------
>From the comment of `getTypePtr()`:

> This function requires that the type not be NULL. If the type might be NULL, use the (slightly less efficient) \c getTypePtrOrNull().`

I think we should use `getTypePtrOrNull` if you want to check the type ptr is null.

```
if (const auto* Type = TL.getTypePtrOrNull()) 
  if (if (const auto* D = Type.getTagDecl())
    addToken(D->getLocation(), D);
```


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:57
       };
-      struct {
+      $Class[[struct]] {
       } $Variable[[S]];
----------------
just realize this case, this seems weird to me, we are highlighting a keyword `struct` as a class type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64257