hokein added a comment.
thanks, looks good, just a few nits.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:34
+ return true;
+ if (!Name.isIdentifier())
+ return false;
----------------
nit: I think the check is redundant, getAsIdentifierInfo() will return nullptr
if `!Name.isIdentifier()`.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:37
+ auto *II = Name.getAsIdentifierInfo();
+ return II && II->getName() != "";
+}
----------------
nit: `!II->getName().empty()`
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:169
private:
- void addTokenForTypedef(SourceLocation Loc, const TypedefNameDecl *TD) {
- auto *TSI = TD->getTypeSourceInfo();
- if (!TSI)
- return;
- // Try to highlight as underlying type.
- if (addType(Loc, TSI->getType().getTypePtrOrNull()))
- return;
- // Fallback to the typedef highlighting kind.
- addToken(Loc, HighlightingKind::Typedef);
- }
-
- bool addType(SourceLocation Loc, const Type *TP) {
+ llvm::Optional<HighlightingKind> kindForType(const Type *TP) {
if (!TP)
----------------
how about moving out this method (and `kindForDecl`) of the class, they seem to
not depend on any fields of the class?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:212
+ return HighlightingKind::Parameter;
+ if (const VarDecl *VD = dyn_cast<VarDecl>(D))
+ return VD->isStaticDataMember()
----------------
nit: auto
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:260
+ void addToken(SourceLocation Loc, const NamedDecl *D) {
+ auto K = kindForDecl(D);
+ if (!K)
----------------
maybe
```
if (auto K = kindForDecl(D))
addToken(Loc, *K);
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67341/new/
https://reviews.llvm.org/D67341
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
