[PATCH] D63559: [clang-tidy] Added functionality for getting semantic highlights for variable and function declarations
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 24 07:32:14 PDT 2019
hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:11
+
+// Collects all semantic symbols in an ASTContext. Symbols on line i are always
+// in front of symbols on line i+1
----------------
The comment is out of date now.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:39
+ AST.getLangOpts());
+ if (Len == 0) {
+ // Don't add symbols that don't have any length.
----------------
We can check the name of `NamedDecl` is empty rather than detecting the length.
Could you add a test for this?
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:75
+ SemanticSymbolASTCollector Collector(Ctx);
+ Collector.TraverseAST(Ctx);
+ return Collector.getSymbols();
----------------
let's move the above lines into `SemanticSymbolASTCollector`, we can define a new method "collectTokens()".
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:9
+//
+// Code for collecting semantic symbols from the C++ AST using the
+// RecursiveASTVisitor
----------------
this comment doesn't provide much information IMO, let's remove it.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:29
+struct SemanticToken {
+ SemanticToken() = default;
+ SemanticToken(SemanticHighlightKind Kind, Range R) : Kind(Kind), R(R) {}
----------------
We can get rid of the default constructor and the constructor below.
================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:38
+
+// Returns semantic highlights for the AST. The vector is ordered in ascending
+// order by the line number. Every symbol in LineHighlight is ordered in
----------------
The comment is out of date.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:31
+ void $foo[[foo]](int $a[[a]]) {
+ auto $vlvn[[VeryLongVariableName]] = 12312;
+ A $aa[[aa]];
----------------
Thinking more about the test, I think we could improve the test to make it more scalable and less verbose code.
How about associating the range name (e.g. $vlan) in the annotation with the name in `SemanticHighlightKind`? For example, in this test case, we could use annotation like
```
void $Function[foo](int $Variable[[a]]) {
auto $Variable[[A]] = 222;
}
```
We have a common function like `checkHighlights` which verifies that all `Functions` and `Variable` ranges.
```
void checkHighlights(... annotation) {
auto ActualHighlights = getHighlights(annotation.code());
EXPECT_THAT(annotations.ranges('Function'), unorderedElemenetsAre(filter(ActualHighlights, SemanticHighlightKind::Function));
EXPECT_THAT(annotations.ranges('Variable'), unorderedElemenetsAre(filter(ActualHighlights, SemanticHighlightKind::Variable))
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63559/new/
https://reviews.llvm.org/D63559
More information about the cfe-commits
mailing list