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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, I think we can simplify the interface further, see my comments inline.

The boundary of this patch seems unclear, currently it contains C++ APIs, and some implementations for semantic highlighting LSP. I think we should merely focus on the C++ APIs in this patch.



================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:32
+  void addSymbol(Decl *D, SemanticScope Scope) {
+    auto Loc = D->getLocation();
+    SemanticToken S;
----------------
Looks like the code doesn't handle the case where D is expanded from a macro (Loc is a macro location).


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:129
+std::vector<std::vector<std::string>> getSemanticScopes() {
+  return {{"variable"}, {"entity.name.function"}};
+}
----------------
This is Textmate-specific, I think we should lift it to a new File (TextMate.cpp). We can do it in a separate patch.

I'd use a map to explicitly express the relationship between `SemanticScope` and TextMate scope, the current implementation is not obvious.

```
{SemanticScope::Function, "entity.name.function"},
{SemanticScope::Variable, "variable.other"},
```


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:133
+std::vector<LineHighlight> getASTHighlights(ASTContext &AST) {
+  SemanticSymbolASTCollector Collector(AST);
+  Collector.TraverseAST(AST);
----------------
We should only collect the highlights from the main AST, I think we should set traversal scope only to `localTopDecls` (use `ParsedAST` as parameter would help here)

`ASTCtx.setTraversalScope(MainAST.getLocalTopLevelDecls());` 


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:14
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICSYMBOLASTCOLLECTOR_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICSYMBOLASTCOLLECTOR_H
----------------
nit: the name of header guard doesn't reflect the file name.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:21
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Lex/Lexer.h"
+
----------------
please clean-up your #includes:
-  Headers.h is unused
- we use RecursiveASTVisitor, Lexer in the .cpp file, we should include these headers in the .cpp file rather the `.h` file.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:26
+
+// ScopeIndex represents the mapping from the scopes list to a type of
+// expression.
----------------
The comment seems not very related to this structure.

We'd use this `enum` class to find a corresponding `TextMate` scope in the future, but this is implementation detail, the comment here should mention the high-level things about this structure.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:28
+// expression.
+enum class SemanticScope : int {
+  VariableDeclaration = 0,
----------------
TextMate is using the term `scope` for different tokens, but the scope has different meaning in C++ world (usually the namespace "ns::" of a symbol). I'd avoid using this word in the C++ interface.

Just `SemanticHighlightingKind`?


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:29
+enum class SemanticScope : int {
+  VariableDeclaration = 0,
+  FunctionDeclaration = 1,
----------------
not exactly sure whether we should distinguish these cases at the moment (function declaration vs function calls, variable declaration vs variable references).

I think we could just use `Variable`, `Function` at the beginning, and add more kinds afterwards.




================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:39
+  SemanticScope Scope;
+  Position StartPosition;
+  unsigned int Len;
----------------
instead of using `start position + length`, I'd use `Range R;`


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:61
+// ascending order by their coumn number.
+std::vector<LineHighlight> getASTHighlights(ASTContext &AST);
+std::vector<std::vector<std::string>> getSemanticScopes();
----------------
I'd drop the `AST` word, how about naming it "getSemanticHighlights"?

I think we can just return "vector<HighlightingToken>", and we could we could group them by line when returning a LSP result. 

The function parameter should be `ParsedAST`.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63559/new/

https://reviews.llvm.org/D63559