[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.

the test looks better now, another round of reviews, most are nits.



================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:19
+class SemanticTokenCollector
+    : private RecursiveASTVisitor<SemanticTokenCollector> {
+  friend class RecursiveASTVisitor<SemanticTokenCollector>;
----------------
nit: public.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:20
+    : private RecursiveASTVisitor<SemanticTokenCollector> {
+  friend class RecursiveASTVisitor<SemanticTokenCollector>;
+  std::vector<SemanticToken> Tokens;
----------------
do we need friend declaration now?


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:28
+      : Ctx(AST.getASTContext()), SM(AST.getSourceManager()) {
+    Ctx.setTraversalScope(AST.getLocalTopLevelDecls());
+  }
----------------
I'd move this line to `collectTokens` as they are related.

As discussed, setTraversalScope doesn't guarantee that we wouldnot visit Decl* outside of the main file (some decls are still reachable), so we may get tokens outside of the main file. If you don't address it in this patch, that's fine, leave a FIXME here.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:32
+    TraverseAST(Ctx);
+    return Tokens;
+  }
----------------
add `assume(Tokens.empty())?`, if we call this method twice, we will accumulate the `Tokens`.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:45
+
+  void addSymbol(NamedDecl *D, SemanticHighlightKind Kind) {
+    if (D->getLocation().isMacroID()) {
----------------
addSymbol => addToken;
NamedDecl *D => const NamedDecl* D


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:47
+    if (D->getLocation().isMacroID()) {
+      // FIXME; This is inside a macro. For now skip the token
+      return;
----------------
nit: `FIXME: skip tokens inside macros for now.`


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:51
+
+    if (D->getName().size() == 0) {
+      // Don't add symbols that don't have any length.
----------------
use getDeclName().isEmpty().


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:56
+
+    auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+    if (!R.hasValue()) {
----------------
How about pulling out a function `llvm::Optional<SemanticToken> makeSemanticToken(..)`?


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:59
+      // R should always have a value, if it doesn't something is very wrong.
+      llvm_unreachable("Tried to add semantic token with an invalid range");
+    }
----------------
this would crash clangd if we meet this case, I think we could just emit a log. 


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:14
+#include "Protocol.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
----------------
nit: remove the unused include.


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:19
+
+enum class SemanticHighlightKind : int {
+  Variable = 0,
----------------
Maybe just

```
enum SemanticHighlightKind {
   Variable,
   Function
};
```


================
Comment at: clang-tools-extra/clangd/SemanticHighlight.h:39
+#endif
\ No newline at end of file

----------------
add a new line.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:11
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "SemanticHighlight.h"
----------------
not used #include


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:15
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
----------------
is this #include used?


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:24
+
+std::vector<SemanticToken> makeSemanticTokens(std::vector<Range> Ranges,
+                                             SemanticHighlightKind Kind) {
----------------
we are passing a copy here, use llvm::ArrayRef.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:27
+  std::vector<SemanticToken> Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; I++) {
+    Tokens[I].R = Ranges[I];
----------------
nit: ++I


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:35
+
+void checkHighlights(std::string Code) {
+
----------------
nit: llvm::StringRef


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:39
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::map<SemanticHighlightKind, std::string> KindToString{
+      {SemanticHighlightKind::Variable, "Variable"},
----------------
static const 


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:43
+  std::vector<SemanticToken> ExpectedTokens;
+  for (auto KindString : KindToString) {
+    std::vector<SemanticToken> Toks =
----------------
nit: const auto&


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:53
+
+TEST(SemanticTokenCollector, GetBeginningOfIdentifier) {
+  checkHighlights(R"cpp(
----------------
nit: update the test name.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:65
+
+TEST(SemanticTokenCollector, DoesNotGetUnnamedParamDecls) {
+  checkHighlights(R"cpp(
----------------
I think we can group these two testcases like

```
const char* TestCases[] =  {
   {},
   {}, 
};

for (...) {
}
```


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:66
+TEST(SemanticTokenCollector, DoesNotGetUnnamedParamDecls) {
+  checkHighlights(R"cpp(
+    void $Function[[foo]](int);
----------------
maybe use another obvious case:

```
struct {
} name;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559