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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
