[PATCH] D63559: [clang-tidy] Added functionality for getting semantic highlights for variable and function declarations

[Haojian Wu via Phabricator via cfe-commits]

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