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