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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

could you please also update the patch description? "non-builtin" types are not precise, this patch only highlights the class and enum types.



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:71
+
+    TagDecl *D = TL.getTypePtr()->getAsTagDecl();
+    if (!D)
----------------
nit: you could simplify the code like 

```
if (const auto* D = TL.getXXX)
  addToken(D->getLocation(), D);
return true;
```


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:81
   void addToken(SourceLocation Loc, const Decl *D) {
+    if (isa<CXXConstructorDecl>(D)) {
+      addToken(Loc, HighlightingKind::Class);
----------------
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`.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:87
+      typedef abc::$Class[[A]]<int> AAA;
+      enum class $Enum[[E]] {};
+      enum $Enum[[EE]] {};
----------------
could we split the enum case into a separate testcase? 

Thinking it further, we may want to highlight the enumerator as well.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:101
+    R"cpp(
+      struct $Class[[A]] {
+        $Class[[A]]();
----------------
this test case can be merged into the above case (we could add constructor/destructor to class B there)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64257