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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:68
+
+    TagDecl *D = TL.getTypePtr()->getAsTagDecl();
+    if (!D)
----------------
jvikstrom wrote:
> hokein wrote:
> > We are only interested in `TagDecl`, maybe use the `VisitTagLoc` callback, so that you can get rid of the filtering code above.
> With just VisitTagLoc it does not catch this case: 
> ```
> namespace abc {
>   template<typename T>
>   struct $Type[[A]] {};
> }
> abc::$Type[[A]]<int> $Variable[[AA]];```
> 
> I guess I could add a bunch of ```Visit*TypeLoc``` methods but I can't seem to find the correct Visit method for the case above... 
Your logic seems sound to me, but please add a comment like "This covers classes, class templates, etc"


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:99
     }
+    if(isa<CXXRecordDecl>(D)) {
+      addToken(Loc, HighlightingKind::Type);
----------------
hokein wrote:
> nit: clang-format
RecordDecl, unless you're trying to distinguish C++ from C for some reason


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:61
+  bool VisitTypeLoc(TypeLoc &TL) {
+    // The check for DependentName is so namespace qualifiers are not
+    // highlighted. The check for elaborated type is for not getting two entries
----------------
should you highlight DependentNameTypeLoc::getNameLoc()? (As some generic "type" rather than "class" as we can't resolve the name) Or is the unqualified name highlighted by traversing some other node?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:62
+    // The check for DependentName is so namespace qualifiers are not
+    // highlighted. The check for elaborated type is for not getting two entries
+    // whenever there is an anonymous struct.
----------------
Nit: consider splitting this || into two statements so you can comment each.

The comments should ideally say what drives the actual behavior in this situation, e.g. "For elaborated types, the underlying type will be highlighted when visiting the inner typeloc"


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:70
+    TagDecl *D = TL.getTypePtr()->getAsTagDecl();
+    if (!D)
+      return true;
----------------
The patch description says "non-builtin types".

It's fine to just handle tag types (struct class enum union) in this patch, but there are other cases you may want to handle later (e.g. typedefs/using).

Random thought for the future: you could highlight `auto` differently based on the actual underlying type (e.g. class vs enum vs pointer...) 


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:73
+
+    if (const auto *RD = dyn_cast<CXXRecordDecl>(D)) {
+      if (const auto *DC = RD->getDestructor()) {
----------------
This code is doing something really weird: you've seen a mention of a class, now you're checking to see if the destructor's declaration encloses the mention so you can not highlight it.

First, I'm not convinced highlighting it is bad or worth any complexity to avoid, happy to discuss further.

Second, this will get a bunch of cases wrong:
 - a call to the destructor (`a->~Foo()`) will still be highligthed
 - the out-of-line definition of the destructor (`Foo::~Foo() { ... }`) will still be highlighted
 - a (separate) mention of the class name within the body of the destructor (defined inline) will not be highlighted


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:100
     }
+    if (isa<CXXRecordDecl>(D)) {
+      addToken(Loc, HighlightingKind::Class);
----------------
consider implementing `enum` in this patch too: it's a TagDecl so you've already done almost all the work, just need to add another if here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64257