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