[PATCH] D63559: [clangd] Added functionality for getting semantic highlights for variable and function declarations

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks mostly good. Thinking more about the name, we should align with the LSP proposal, see my comments inline.



================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:20
+// Collects all semantic tokens in an ASTContext.
+class SemanticTokenCollector
+    : public RecursiveASTVisitor<SemanticTokenCollector> {
----------------
HighlightingTokenCollector.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:17
+
+enum class SemanticHighlightKind {
+  Variable,
----------------
LSP proposal is using `Highlighting` rather than `Highlight`, let's align with the LSP proposal, using  `Highlighting` in our names (comments, function, class, and files).


The name seems too verbose, let's drop the `Semantic`, just use `HighlightingKind`.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:23
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
----------------
here use `HighlightingToken`.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:32
+// main AST.
+std::vector<SemanticToken> getSemanticHighlights(ParsedAST &AST);
+
----------------
getSemanticHighlightings.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:46
+
+TEST(SemanticTokenCollector, GetsCorrectTokens) {
+  const char *TestCases[] = {
----------------
nit: SemanticTokenCollector => SemanticHighlighting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559