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