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

[Haojian Wu via Phabricator via cfe-commits]

hokein closed this revision.
hokein added a comment.

the patch was landed in rL364421 , not sure 
why it was not associated with this review.


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

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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks, looks good.




Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:1
+//==- SemanticHighlightingTest.cpp - SemanticHighlightTest tests-*- C++ -* 
-==//
+//

nit: SemanticHighlightingTest.cpp => SemanticHighlightingTests.cpp



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:18
+
+std::vector makeHighlightingTokens(llvm::ArrayRef 
Ranges,
+  HighlightingKind Kind) {

nit: clang-format


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

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 206630.
jvikstrom marked 6 inline comments as done.
jvikstrom added a comment.

Renamed types to follow LSP. Also renamed files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -0,0 +1,69 @@
+//==- SemanticHighlightingTest.cpp - SemanticHighlightTest tests-*- C++ -* -==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SemanticHighlighting.h"
+#include "TestTU.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::vector makeHighlightingTokens(llvm::ArrayRef Ranges,
+  HighlightingKind Kind) {
+  std::vector Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; ++I) {
+Tokens[I].R = Ranges[I];
+Tokens[I].Kind = Kind;
+  }
+
+  return Tokens;
+}
+
+void checkHighlightings(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  static const std::map KindToString{
+  {HighlightingKind::Variable, "Variable"},
+  {HighlightingKind::Function, "Function"}};
+  std::vector ExpectedTokens;
+  for (const auto  : KindToString) {
+std::vector Toks =
+makeHighlightingTokens(Test.ranges(KindString.second), KindString.first);
+ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
+  }
+
+  auto ActualTokens = getSemanticHighlightings(AST);
+  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+}
+
+TEST(SemanticHighlighting, GetsCorrectTokens) {
+  const char *TestCases[] = {
+  R"cpp(
+struct A {
+  double SomeMember;
+};
+struct {
+}   $Variable[[HStruct]];
+void $Function[[foo]](int $Variable[[a]]) {
+  auto $Variable[[VeryLongVariableName]] = 12312;
+  A $Variable[[aa]];
+}
+  )cpp",
+  R"cpp(
+void $Function[[foo]](int);
+  )cpp"};
+  for (const auto  : TestCases) {
+checkHighlightings(TestCase);
+  }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightingTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -0,0 +1,37 @@
+//==-- SemanticHighlighting.h - Generating highlights from the AST-- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+
+namespace clang {
+namespace clangd {
+
+enum class HighlightingKind {
+  Variable,
+  Function,
+};
+
+// Contains all information needed for the highlighting a token.
+struct HighlightingToken {
+  HighlightingKind Kind;
+  Range R;
+};
+
+bool operator==(const HighlightingToken , const HighlightingToken );
+
+// Returns all HighlightingTokens from an AST. Only generates highlights for the
+// main AST.
+std::vector getSemanticHighlightings(ParsedAST );
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -0,0 +1,78 @@
+//===--- SemanticHighlighting.cpp - -- ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlight.h:17
+
+enum class SemanticHighlightKind {
+  Variable,

jvikstrom wrote:
> hokein wrote:
> > 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`.
> There is also a `DocumentHighlightingKind` enum though. Is there not a 
> possibility of  confusing people with `HighlightingKind` and 
> `DocumentHighlightingKind`?
We have a `DocumentHighlightKind` (without `ing`), but that is in the LSP 
layer. 

Yeah, it would not be super clear, but if we are in the related context, 
`HighlightingKind` is clear -- ClangdServer will use `getSemanticHighlights` to 
get all highlight tokens, so I'd prefer shorter names.


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

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom marked an inline comment as done.
jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlight.h:17
+
+enum class SemanticHighlightKind {
+  Variable,

hokein wrote:
> 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`.
There is also a `DocumentHighlightingKind` enum though. Is there not a 
possibility of  confusing people with `HighlightingKind` and 
`DocumentHighlightingKind`?


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

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

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 getSemanticHighlights(ParsedAST );
+

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added inline comments.



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

hokein wrote:
> the code seems not clang-format, could you run clang-format on your patch?
It has been clang-formated. The spaces are part of the test (need to check that 
the range does not include spaces even if there are multiple ones)


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

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 206618.
jvikstrom marked 11 inline comments as done.
jvikstrom added a comment.

- [clangd] Added functionality for getting semantic highlights for variable and 
function declarations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticHighlight.cpp
  clang-tools-extra/clangd/SemanticHighlight.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
@@ -0,0 +1,69 @@
+//===- SemanticHighlightTest.cpp - SemanticHighlightTest tests-*- C++ -* -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SemanticHighlight.h"
+#include "TestTU.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::vector makeSemanticTokens(llvm::ArrayRef Ranges,
+  SemanticHighlightKind Kind) {
+  std::vector Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; ++I) {
+Tokens[I].R = Ranges[I];
+Tokens[I].Kind = Kind;
+  }
+
+  return Tokens;
+}
+
+void checkHighlights(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  static const std::map KindToString{
+  {SemanticHighlightKind::Variable, "Variable"},
+  {SemanticHighlightKind::Function, "Function"}};
+  std::vector ExpectedTokens;
+  for (const auto  : KindToString) {
+std::vector Toks =
+makeSemanticTokens(Test.ranges(KindString.second), KindString.first);
+ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
+  }
+
+  auto ActualTokens = getSemanticHighlights(AST);
+  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+}
+
+TEST(SemanticTokenCollector, GetsCorrectTokens) {
+  const char *TestCases[] = {
+  R"cpp(
+struct A {
+  double SomeMember;
+};
+struct {
+}   $Variable[[HStruct]];
+void $Function[[foo]](int $Variable[[a]]) {
+  auto $Variable[[VeryLongVariableName]] = 12312;
+  A $Variable[[aa]];
+}
+  )cpp",
+  R"cpp(
+void $Function[[foo]](int);
+  )cpp"};
+  for (const auto  : TestCases) {
+checkHighlights(TestCase);
+  }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/SemanticHighlight.h
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.h
@@ -0,0 +1,37 @@
+//==-- SemanticHighlight.h - Generating highlights from the AST--*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+
+namespace clang {
+namespace clangd {
+
+enum class SemanticHighlightKind {
+  Variable,
+  Function,
+};
+
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
+  Range R;
+};
+
+bool operator==(const SemanticToken , const SemanticToken );
+
+// Returns all SemanticTokens from an AST. Only generates highlights for the
+// main AST.
+std::vector getSemanticHighlights(ParsedAST );
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/SemanticHighlight.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.cpp
@@ -0,0 +1,78 @@
+//===--- SemanticHighlight.cpp - -- -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license 

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

much clearer now, a few more nits.




Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:69
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
+//FIXME: There is an edge case where this will still get decls from 
include files.
 for (Decl *D : DG) {

not need to add this in this patch as you are also working on a fix in a 
separate patch.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:56
+
+auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+if (!R.hasValue()) {

jvikstrom wrote:
> hokein wrote:
> > How about pulling out a function `llvm::Optional 
> > makeSemanticToken(..)`?
> Don't understand what you mean.
I meant we could move lines 56 ~ 66 to a new function, the current 
implementation is fine (as `addToken` is small).



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

nit: remove the `{}` if the body contains only one statement, the same to other 
places.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:57
+auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+if (!R.hasValue()) {
+  // R should always have a value, if it doesn't something is very wrong.

nit: just "if (!R)" would work.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:64
+SemanticToken S;
+S.R = R.getValue();
+S.Kind = Kind;

maybe just `Tokens.emplace_back(Kind, *R)`.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:75
+}
+bool operator!=(const SemanticToken , const SemanticToken ) {
+  return !(Lhs == Rhs);

we probably don't need the `!=` operator in this patch.



Comment at: clang-tools-extra/clangd/SemanticHighlight.h:13
+#include "ClangdUnit.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+

the RecursiveASTVisitor.h should be in the .cpp file.



Comment at: clang-tools-extra/clangd/SemanticHighlight.h:32
+
+std::vector getSemanticHighlights(ParsedAST );
+

needs high-level comments.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:24
+
+std::vector makeSemanticTokens(std::vector Ranges,
+ SemanticHighlightKind Kind) {

jvikstrom wrote:
> hokein wrote:
> > we are passing a copy here, use llvm::ArrayRef.
> I changed to pass a const vector & instead. Is that alright as well?
yeah, that works as well, llvm prefers `llvm::ArrayRef`, but up to you.



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

the code seems not clang-format, could you run clang-format on your patch?


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

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:28
+  : Ctx(AST.getASTContext()), SM(AST.getSourceManager()) {
+Ctx.setTraversalScope(AST.getLocalTopLevelDecls());
+  }

hokein wrote:
> 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.
Will fix in a separate patch for topLevelDecls. Don't really know what FIXME to 
add  here. Added one to getLocalTopLevelDecls. Don't really have a good 
understanding of the reason as to what happens yet so the FIXME is very general 
(as you can probably tell from it)...



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:56
+
+auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+if (!R.hasValue()) {

hokein wrote:
> How about pulling out a function `llvm::Optional 
> makeSemanticToken(..)`?
Don't understand what you mean.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:24
+
+std::vector makeSemanticTokens(std::vector Ranges,
+ SemanticHighlightKind Kind) {

hokein wrote:
> we are passing a copy here, use llvm::ArrayRef.
I changed to pass a const vector & instead. Is that alright as well?


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

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 206593.
jvikstrom marked 24 inline comments as done.
jvikstrom added a comment.

Fixed a bunch of small comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/SemanticHighlight.cpp
  clang-tools-extra/clangd/SemanticHighlight.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
@@ -0,0 +1,68 @@
+//===- SemanticHighlightTest.cpp - SemanticHighlightTest tests-*- C++ -* -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "SemanticHighlight.h"
+#include "TestTU.h"
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::vector makeSemanticTokens(const std::vector ,
+  SemanticHighlightKind Kind) {
+  std::vector Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; ++I) {
+Tokens[I].R = Ranges[I];
+Tokens[I].Kind = Kind;
+  }
+
+  return Tokens;
+}
+
+void checkHighlights(llvm::StringRef Code) {
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  static const std::map KindToString{
+  {SemanticHighlightKind::Variable, "Variable"},
+  {SemanticHighlightKind::Function, "Function"}};
+  std::vector ExpectedTokens;
+  for (const auto  : KindToString) {
+std::vector Toks =
+makeSemanticTokens(Test.ranges(KindString.second), KindString.first);
+ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
+  }
+
+  auto ActualTokens = getSemanticHighlights(AST);
+  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+}
+
+TEST(SemanticTokenCollector, GetsCorrectTokens) {
+  const char *TestCases[] = {
+  R"cpp(
+struct A {
+  double SomeMember;
+};
+struct {}   $Variable[[HStruct]];
+void $Function[[foo]](int $Variable[[a]]) {
+  auto $Variable[[VeryLongVariableName]] = 12312;
+  A $Variable[[aa]];
+}
+  )cpp",
+  R"cpp(
+void $Function[[foo]](int);
+  )cpp"};
+  for (const auto  : TestCases) {
+checkHighlights(TestCase);
+  }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/SemanticHighlight.h
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.h
@@ -0,0 +1,37 @@
+//==-- SemanticHighlight.h - Generating highlights from the AST--*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+namespace clang {
+namespace clangd {
+
+enum class SemanticHighlightKind {
+  Variable,
+  Function,
+};
+
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
+  Range R;
+};
+
+bool operator==(const SemanticToken , const SemanticToken );
+bool operator!=(const SemanticToken , const SemanticToken );
+
+std::vector getSemanticHighlights(ParsedAST );
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/SemanticHighlight.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.cpp
@@ -0,0 +1,85 @@
+//===--- SemanticHighlight.cpp - -- -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// 

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 206450.
jvikstrom added a comment.

Made SemanticTokenCollector skip Decls not in the main file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticHighlight.cpp
  clang-tools-extra/clangd/SemanticHighlight.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
@@ -0,0 +1,73 @@
+//===- SemanticHighlightTest.cpp - SemanticHighlightTest tests-*- C++ -* -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "SemanticHighlight.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::vector makeSemanticTokens(std::vector Ranges,
+ SemanticHighlightKind Kind) {
+  std::vector Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; I++) {
+Tokens[I].R = Ranges[I];
+Tokens[I].Kind = Kind;
+  }
+
+  return Tokens;
+}
+
+void checkHighlights(std::string Code) {
+
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::map KindToString{
+  {SemanticHighlightKind::Variable, "Variable"},
+  {SemanticHighlightKind::Function, "Function"}};
+  std::vector ExpectedTokens;
+  for (auto KindString : KindToString) {
+std::vector Toks =
+makeSemanticTokens(Test.ranges(KindString.second), KindString.first);
+ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
+  }
+
+  auto ActualTokens = getSemanticHighlights(AST);
+  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+}
+
+TEST(SemanticTokenCollector, GetBeginningOfIdentifier) {
+  checkHighlights(R"cpp(
+struct A {
+  double SomeMember;
+};
+void $Function[[foo]](int $Variable[[a]]) {
+  auto $Variable[[VeryLongVariableName]] = 12312;
+  A $Variable[[aa]];
+}
+  )cpp");
+}
+
+TEST(SemanticTokenCollector, DoesNotGetUnnamedParamDecls) {
+  checkHighlights(R"cpp(
+void $Function[[foo]](int);
+  )cpp");
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/SemanticHighlight.h
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.h
@@ -0,0 +1,38 @@
+//==-- SemanticHighlight.h - Generating highlights from the AST--*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+namespace clang {
+namespace clangd {
+
+enum class SemanticHighlightKind : int {
+  Variable = 0,
+  Function = 1,
+};
+
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
+  Range R;
+};
+
+bool operator==(const SemanticToken , const SemanticToken );
+bool operator!=(const SemanticToken , const SemanticToken );
+
+std::vector getSemanticHighlights(ParsedAST );
+
+} // namespace clangd
+} // namespace clang
+
+#endif
\ No newline at end of file
Index: clang-tools-extra/clangd/SemanticHighlight.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.cpp
@@ -0,0 +1,94 @@
+//===--- SemanticHighlight.cpp - -- -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the 

[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 {
+  friend class RecursiveASTVisitor;

nit: public.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:20
+: private RecursiveASTVisitor {
+  friend class RecursiveASTVisitor;
+  std::vector 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 
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 makeSemanticTokens(std::vector Ranges,
+ SemanticHighlightKind Kind) {

we are passing a copy here, use llvm::ArrayRef.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:27
+  std::vector 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 KindToString{
+  {SemanticHighlightKind::Variable, "Variable"},

static const 



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

nit: const auto&



Comment at: 

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:57
+
+  checkTokensExists(Tokens, Variables, SemanticHighlightKind::Variable);
+  checkTokensExists(Tokens, Function, SemanticHighlightKind::Function);

hokein wrote:
> 
> ```
> checkHighlights(R"cpp(void $Function[[Foo]]))cpp");
> 
> checkHighlights(...) {
>...
>ExpectedTokens = 
> annotation.ranges(toString(SemanticHighlightKind::Variable));
>ExpectedTokens = 
> annotation.ranges(toString(SemanticHighlightKind::Function));
>EXPECT_THAT(ExpectedTokens, UnorderedElementsAreArray(ActualTokens));
> }
Had to add some extra logic to merge the different token types


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

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 206393.
jvikstrom marked 4 inline comments as done.
jvikstrom added a comment.

Changed tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticHighlight.cpp
  clang-tools-extra/clangd/SemanticHighlight.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
@@ -0,0 +1,73 @@
+//===- SemanticHighlightTest.cpp - SemanticHighlightTest tests-*- C++ -* -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "SemanticHighlight.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::vector makeSemanticTokens(std::vector Ranges,
+ SemanticHighlightKind Kind) {
+  std::vector Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; I++) {
+Tokens[I].R = Ranges[I];
+Tokens[I].Kind = Kind;
+  }
+
+  return Tokens;
+}
+
+void checkHighlights(std::string Code) {
+
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::map KindToString{
+  {SemanticHighlightKind::Variable, "Variable"},
+  {SemanticHighlightKind::Function, "Function"}};
+  std::vector ExpectedTokens;
+  for (auto KindString : KindToString) {
+std::vector Toks =
+makeSemanticTokens(Test.ranges(KindString.second), KindString.first);
+ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
+  }
+
+  auto ActualTokens = getSemanticHighlights(AST);
+  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+}
+
+TEST(SemanticTokenCollector, GetBeginningOfIdentifier) {
+  checkHighlights(R"cpp(
+struct A {
+  double SomeMember;
+};
+void $Function[[foo]](int $Variable[[a]]) {
+  auto $Variable[[VeryLongVariableName]] = 12312;
+  A $Variable[[aa]];
+}
+  )cpp");
+}
+
+TEST(SemanticTokenCollector, DoesNotGetUnnamedParamDecls) {
+  checkHighlights(R"cpp(
+void $Function[[foo]](int);
+  )cpp");
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/SemanticHighlight.h
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.h
@@ -0,0 +1,38 @@
+//==-- SemanticHighlight.h - Generating highlights from the AST--*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+namespace clang {
+namespace clangd {
+
+enum class SemanticHighlightKind : int {
+  Variable = 0,
+  Function = 1,
+};
+
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
+  Range R;
+};
+
+bool operator==(const SemanticToken , const SemanticToken );
+bool operator!=(const SemanticToken , const SemanticToken );
+
+std::vector getSemanticHighlights(ParsedAST );
+
+} // namespace clangd
+} // namespace clang
+
+#endif
\ No newline at end of file
Index: clang-tools-extra/clangd/SemanticHighlight.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.cpp
@@ -0,0 +1,83 @@
+//===--- SemanticHighlight.cpp - -- -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:43
+
+  unsigned int Len =
+  clang::Lexer::MeasureTokenLength(D->getLocation(), SM, 
Ctx.getLangOpts());

we can reuse the getTokenRange() function (in SourceCode.h), you need to sync 
your branch to master.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:46
+
+  if (D->getLocation().isMacroID()) {
+// FIXME; This is inside a macro. For now skip the token

I'd move this line at the beginning of the method.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:57
+
+  checkTokensExists(Tokens, Variables, SemanticHighlightKind::Variable);
+  checkTokensExists(Tokens, Function, SemanticHighlightKind::Function);


```
checkHighlights(R"cpp(void $Function[[Foo]]))cpp");

checkHighlights(...) {
   ...
   ExpectedTokens = 
annotation.ranges(toString(SemanticHighlightKind::Variable));
   ExpectedTokens = 
annotation.ranges(toString(SemanticHighlightKind::Function));
   EXPECT_THAT(ExpectedTokens, UnorderedElementsAreArray(ActualTokens));
}


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

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 206376.
jvikstrom marked 3 inline comments as done.
jvikstrom added a comment.

Added header and empty line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticHighlight.cpp
  clang-tools-extra/clangd/SemanticHighlight.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
@@ -0,0 +1,63 @@
+//===- SemanticHighlightTest.cpp - SemanticHighlightTest tests-*- C++ -* -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "SemanticHighlight.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+void checkTokensExists(std::vector Tokens,
+   std::vector ExpectedRanges,
+   SemanticHighlightKind Kind) {
+  std::vector ActualRanges;
+  for (SemanticToken Token : Tokens) {
+if (Token.Kind == Kind) {
+  ActualRanges.push_back(Token.R);
+}
+  }
+
+  EXPECT_THAT(ActualRanges, testing::UnorderedElementsAreArray(ExpectedRanges));
+}
+
+TEST(SemanticTokenCollector, GetBeginningOfIdentifier) {
+  std::string Preamble = R"cpp(
+void $Function[[foo]](int);
+struct A {
+  double SomeMember;
+};
+void $Function[[foo]](int $Variable[[a]]) {
+  SOMEDECL( );
+  auto $Variable[[VeryLongVariableName]] = 12312;
+  A $Variable[[aa]];
+}
+  )cpp";
+
+  Annotations Test(Preamble);
+  auto Variables = Test.ranges("Variable");
+  auto Function = Test.ranges("Function");
+
+  auto AST = TestTU::withCode(Test.code()).build();
+  auto Tokens = SemanticTokenCollector(AST).collectTokens();
+
+  checkTokensExists(Tokens, Variables, SemanticHighlightKind::Variable);
+  checkTokensExists(Tokens, Function, SemanticHighlightKind::Function);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/SemanticHighlight.h
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.h
@@ -0,0 +1,55 @@
+//==-- SemanticHighlight.h - Generating highlights from the AST--*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+namespace clang {
+namespace clangd {
+
+enum class SemanticHighlightKind : int {
+  Variable = 0,
+  Function = 1,
+};
+
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
+  Range R;
+};
+
+// Collects all semantic tokens in an ASTContext.
+class SemanticTokenCollector
+: private RecursiveASTVisitor {
+  friend class RecursiveASTVisitor;
+  std::vector Tokens;
+  ASTContext 
+  const SourceManager 
+
+public:
+  SemanticTokenCollector(ParsedAST );
+  std::vector collectTokens();
+
+private:
+  bool VisitVarDecl(VarDecl *Var);
+  bool VisitFunctionDecl(FunctionDecl *Func);
+
+  void addSymbol(NamedDecl *D, SemanticHighlightKind Kind);
+};
+
+bool operator==(const SemanticToken , const SemanticToken );
+bool operator!=(const SemanticToken , const SemanticToken );
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/SemanticHighlight.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.cpp
@@ -0,0 

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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 206375.
jvikstrom added a comment.

Made SemanticTokenCollector visible and removed getSemanticHighlights function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticHighlight.cpp
  clang-tools-extra/clangd/SemanticHighlight.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
@@ -0,0 +1,63 @@
+//===- SemanticHighlightTest.cpp - SemanticHighlightTest tests-*- C++ -* -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "SemanticHighlight.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+void checkTokensExists(std::vector Tokens,
+   std::vector ExpectedRanges,
+   SemanticHighlightKind Kind) {
+  std::vector ActualRanges;
+  for (SemanticToken Token : Tokens) {
+if (Token.Kind == Kind) {
+  ActualRanges.push_back(Token.R);
+}
+  }
+
+  EXPECT_THAT(ActualRanges, testing::UnorderedElementsAreArray(ExpectedRanges));
+}
+
+TEST(SemanticTokenCollector, GetBeginningOfIdentifier) {
+  std::string Preamble = R"cpp(
+void $Function[[foo]](int);
+struct A {
+  double SomeMember;
+};
+void $Function[[foo]](int $Variable[[a]]) {
+  SOMEDECL( );
+  auto $Variable[[VeryLongVariableName]] = 12312;
+  A $Variable[[aa]];
+}
+  )cpp";
+
+  Annotations Test(Preamble);
+  auto Variables = Test.ranges("Variable");
+  auto Function = Test.ranges("Function");
+
+  auto AST = TestTU::withCode(Test.code()).build();
+  auto Tokens = SemanticTokenCollector(AST).collectTokens();
+
+  checkTokensExists(Tokens, Variables, SemanticHighlightKind::Variable);
+  checkTokensExists(Tokens, Function, SemanticHighlightKind::Function);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/SemanticHighlight.h
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.h
@@ -0,0 +1,54 @@
+//==-- SemanticHighlight.h - Generating highlights from the AST--*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+namespace clang {
+namespace clangd {
+
+enum class SemanticHighlightKind : int {
+  Variable = 0,
+  Function = 1,
+};
+
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
+  Range R;
+};
+
+// Collects all semantic tokens in an ASTContext.
+class SemanticTokenCollector
+: private RecursiveASTVisitor {
+  friend class RecursiveASTVisitor;
+  std::vector Tokens;
+  ASTContext 
+  const SourceManager 
+
+public:
+  SemanticTokenCollector(ParsedAST );
+  std::vector collectTokens();
+
+private:
+  bool VisitVarDecl(VarDecl *Var);
+  bool VisitFunctionDecl(FunctionDecl *Func);
+
+  void addSymbol(NamedDecl *D, SemanticHighlightKind Kind);
+};
+
+bool operator==(const SemanticToken , const SemanticToken );
+bool operator!=(const SemanticToken , const SemanticToken );
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/SemanticHighlight.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.cpp
@@