[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
This revision was automatically updated to reflect the committed changes.
jvikstrom marked 2 inline comments as done.
Closed by commit rL365111: [clangd] Emit publishSemanticHighlighting in LSP if
enabled (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D63919?vs=207783&id=207978#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
clang-tools-extra/trunk/clangd/SemanticHighlighting.h
clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h
@@ -55,6 +55,8 @@
// Implement DiagnosticsConsumer.
void onDiagnosticsReady(PathRef File, std::vector Diagnostics) override;
void onFileUpdated(PathRef File, const TUStatus &Status) override;
+ void onHighlightingsReady(PathRef File,
+ std::vector Highlightings) override;
// LSP methods. Notifications have signature void(const Params&).
// Calls have signature void(const Params&, Callback).
@@ -115,6 +117,9 @@
void reparseOpenedFiles();
void applyConfiguration(const ConfigurationSettings &Settings);
+ /// Sends a "publishSemanticHighlighting" notification to the LSP client.
+ void publishSemanticHighlighting(SemanticHighlightingParams Params);
+
/// Sends a "publishDiagnostics" notification to the LSP client.
void publishDiagnostics(const URIForFile &File,
std::vector Diagnostics);
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -273,6 +273,12 @@
if (!O)
return false;
if (auto *TextDocument = O->getObject("textDocument")) {
+if (auto *SemanticHighlighting =
+TextDocument->getObject("semanticHighlightingCapabilities")) {
+ if (auto SemanticHighlightingSupport =
+ SemanticHighlighting->getBoolean("semanticHighlighting"))
+R.SemanticHighlighting = *SemanticHighlightingSupport;
+}
if (auto *Diagnostics = TextDocument->getObject("publishDiagnostics")) {
if (auto CategorySupport = Diagnostics->getBoolean("categorySupport"))
R.DiagnosticCategory = *CategorySupport;
@@ -1027,5 +1033,22 @@
return OS << toString(Enc);
}
+bool operator==(const SemanticHighlightingInformation &Lhs,
+const SemanticHighlightingInformation &Rhs) {
+ return Lhs.Line == Rhs.Line && Lhs.Tokens == Rhs.Tokens;
+}
+
+llvm::json::Value toJSON(const SemanticHighlightingInformation &Highlighting) {
+ return llvm::json::Object{{"line", Highlighting.Line},
+{"tokens", Highlighting.Tokens}};
+}
+
+llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting) {
+ return llvm::json::Object{
+ {"textDocument", Highlighting.TextDocument},
+ {"lines", std::move(Highlighting.Lines)},
+ };
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
===
--- clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
+++ clang-tools-extra/trunk/clangd/test/semantic-highlighting.test
@@ -0,0 +1,75 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"semanticHighlightingCapabilities":{"semanticHighlighting":true}}},"trace":"off"}}
+---
+# CHECK: "id": 0,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": {
+# CHECK-NEXT:"capabilities": {
+# CHECK-NEXT: "codeActionProvider": true,
+# CHECK-NEXT: "completionProvider": {
+# CHECK-NEXT:"resolveProvider": false,
+# CHECK-NEXT:"triggerCharacters": [
+# CHECK-NEXT: ".",
+# CHECK-NEXT: ">",
+# CHECK-NEXT: ":"
+# CHECK-NEXT:]
+# CHECK-NEXT: },
+# CHECK-NEXT: "declarationProvider": true,
+# CHECK-NEXT: "definitionProvider": true,
+# CHECK-NEXT: "documentFormattingProvider": true,
+# CHECK-NEXT: "documentHighlightProvider": true,
+# CHECK-NEXT: "documentOnTypeFormattingProvider": {
+# CHECK-NEXT:"firstTriggerCharacter": "\n",
+# CHECK-NEXT:"moreTrigge
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
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/ClangdLSPServer.h:59
+ void onHighlightingsReady(PathRef File,
+ std::vector Highlightings)
override;
nit: clang-format.
Comment at: clang-tools-extra/clangd/test/semantic-highlighting.test:36
+# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
nit: we could just verify this field like:
```
# CHECK: "id": 0,
# CHECK: " "semanticHighlighting": {
# CHECK-NEXT:"scopes": [
# CHECK-NEXT: [
# CHECK-NEXT:"variable.cpp"
# CHECK-NEXT: ],
# CHECK-NEXT: [
# CHECK-NEXT:"entity.name.function.cpp"
# CHECK-NEXT: ]
# CHECK-NEXT:]
# CHECK-NEXT: },
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom updated this revision to Diff 207783.
jvikstrom marked 7 inline comments as done.
jvikstrom added a comment.
Addressed comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/test/semantic-highlighting.test
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -8,6 +8,7 @@
#include "Annotations.h"
#include "ClangdServer.h"
+#include "Protocol.h"
#include "SemanticHighlighting.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -66,7 +67,7 @@
}
}
-TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
public:
std::atomic Count = {0};
@@ -90,6 +91,29 @@
ASSERT_EQ(DiagConsumer.Count, 1);
}
+TEST(SemanticHighlighting, toSemanticHighlightingInformation) {
+ auto CreatePosition = [](int Line, int Character) -> Position {
+Position Pos;
+Pos.line = Line;
+Pos.character = Character;
+return Pos;
+ };
+
+ std::vector Tokens{
+ {HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+ {HighlightingKind::Function,
+Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
+ {HighlightingKind::Variable,
+Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+ std::vector ActualResults =
+ toSemanticHighlightingInformation(Tokens);
+ std::vector ExpectedResults = {
+ {1, "AQAEAAA="},
+ {3, "CAAEAAAEAAMAAQ=="}};
+ EXPECT_EQ(ActualResults, ExpectedResults);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -0,0 +1,75 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"semanticHighlightingCapabilities":{"semanticHighlighting":true}}},"trace":"off"}}
+---
+# CHECK: "id": 0,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": {
+# CHECK-NEXT:"capabilities": {
+# CHECK-NEXT: "codeActionProvider": true,
+# CHECK-NEXT: "completionProvider": {
+# CHECK-NEXT:"resolveProvider": false,
+# CHECK-NEXT:"triggerCharacters": [
+# CHECK-NEXT: ".",
+# CHECK-NEXT: ">",
+# CHECK-NEXT: ":"
+# CHECK-NEXT:]
+# CHECK-NEXT: },
+# CHECK-NEXT: "declarationProvider": true,
+# CHECK-NEXT: "definitionProvider": true,
+# CHECK-NEXT: "documentFormattingProvider": true,
+# CHECK-NEXT: "documentHighlightProvider": true,
+# CHECK-NEXT: "documentOnTypeFormattingProvider": {
+# CHECK-NEXT:"firstTriggerCharacter": "\n",
+# CHECK-NEXT:"moreTriggerCharacter": []
+# CHECK-NEXT: },
+# CHECK-NEXT: "documentRangeFormattingProvider": true,
+# CHECK-NEXT: "documentSymbolProvider": true,
+# CHECK-NEXT: "executeCommandProvider": {
+# CHECK-NEXT:"commands": [
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyTweak"
+# CHECK-NEXT:]
+# CHECK-NEXT: },
+# CHECK-NEXT: "hoverProvider": true,
+# CHECK-NEXT: "referencesProvider": true,
+# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
+# CHECK-NEXT: [
+# CHECK-NEXT:"variable.cpp"
+# CHECK-NEXT: ],
+# CHECK-NEXT: [
+# CHECK-NEXT:"entity.name.function.cpp"
+# CHECK-NEXT: ]
+# CHECK-NEXT:]
+# CHECK-NEXT: },
+# CHECK-NEXT: "signatureHelpProvider": {
+# CHECK-NEXT:"triggerCharacters": [
+# CHECK-NEXT: "(",
+# CHECK-NEXT: ","
+# CHECK-NEXT:]
+# CHECK-NEXT: },
+# CHECK-NEXT: "textDocumentSync": 2,
+# CHECK-NEXT: "typeHierarchyProvider": true
+# CHECK-NEXT: "workspaceSymbolProvider": true
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","v
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
hokein added a comment.
mostly good, a few more nits.
Comment at: clang-tools-extra/clangd/Protocol.h:1187
+};
+
+bool operator==(const SemanticHighlightingInformation &Lhs,
nit: remove the blank line to be consistent with the existing style in this
file.
Comment at: clang-tools-extra/clangd/Protocol.h:1199
+};
+
+llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting);
nit: remove this blank line.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:138
+ for (const auto &Line : TokenLines) {
+llvm::SmallVector LineHighlights;
+llvm::raw_svector_ostream OS(LineHighlights);
jvikstrom wrote:
> hokein wrote:
> > The code is a bit tricky here, if I understand the code correctly,
> > `LineHighlights` is the binary data of tokens (each `char` represents a
> > byte).
> > Maybe `LineEncodingTokens`?
> They aren't "fully" encoded yet though, they get encoded after the inner loop
> is done. How about `LineByteTokens`?
>
sounds good.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:121
+toSemanticHighlightingInformation(llvm::ArrayRef Tokens) {
+ if (Tokens.size() == 0) {
+return {};
nit: remove the "{}"
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:138
+ // Writes the token to LineByteTokens in the byte format specified by the
+ // LSP proposal.
+ write32be(Token.R.start.character, OS);
add
```
|<--- 4 bytes --->|<-2 bytes->|<-2 bytes->|
|character | length |index |
```
to the comment, it helps reader to understand how the encode like.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:156
+ std::vector> NestedScopes(Scopes.size());
+ for (const auto &Scope : Scopes) {
+NestedScopes[static_cast(Scope.first)] = Scope.second;
nit: remove the `{}` for one-body statement.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:7
//
+// An implementation of semantic highlighting based on this proposal:
+// https://github.com/microsoft/vscode-languageserver-node/pull/367 in clangd.
nit: this should in a new section:
```
//==-- 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
//===-===//
//
// An implementation
//
//===-===//
```
Comment at: clang-tools-extra/clangd/test/initialize-params.test:3
# Test initialize request parameters with rootUri
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{},"trace":"off"}}
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{"textDocument":{"semanticHighlightingCapabilities":{"semanticHighlighting":true}}},"trace":"off"}}
# CHECK: "id": 0,
How about moving this to the new `semantic-highlighting.test`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom marked an inline comment as done.
jvikstrom added inline comments.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:138
+ for (const auto &Line : TokenLines) {
+llvm::SmallVector LineHighlights;
+llvm::raw_svector_ostream OS(LineHighlights);
hokein wrote:
> The code is a bit tricky here, if I understand the code correctly,
> `LineHighlights` is the binary data of tokens (each `char` represents a byte).
> Maybe `LineEncodingTokens`?
They aren't "fully" encoded yet though, they get encoded after the inner loop
is done. How about `LineByteTokens`?
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:144
+ write32be(Token.R.start.character, OS);
+ write16be(Token.R.end.character - Token.R.start.character, OS);
+ write16be(static_cast(Token.Kind), OS);
hokein wrote:
> jvikstrom wrote:
> > hokein wrote:
> > > if the token is across multiple line, we will emit an ill-formed results.
> > There's a FIXME above (which is where it should probably be handled). A bit
> > unsure how to solve though. If a token is a block comment spanning multiple
> > lines we would need to know the length of every line in the block comment.
> > Probably something that can be solved with the ASTContext or SourceManager
> > but that can't be accessed in this function.
> oh, i missed that FIXME, the FIXME is a bit far away, maybe move it here (now
> we assume the token is always on the same line).
I think that this should probably be handled above (so if a token covers `N`
different lines it would be separated into `N` tokens in the `TokenLines` map.
I don't really see how it would be possible to handle that in this loop
(without basically putting all the smallvectors for every line in another
vector and writing to `Lines` in another loop after the loop is done. But it
feels like that would be way more complex)
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:8
//===--===//
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHTING_H
hokein wrote:
> The semantic highlighting is not in LSP yet, I think we need some
> documentations here to explain some more details about this feature in
> clangd, like the implementations are based on the proposal
> (https://github.com/microsoft/vscode-languageserver-node/pull/367).
Changed the initialize-params test to have semantic highlighting enabled as
well.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom updated this revision to Diff 207758.
jvikstrom marked 8 inline comments as done.
jvikstrom added a comment.
Not sending TextMate scopes over LSP if semantic highlighting is not enabled.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/test/semantic-highlighting.test
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -8,6 +8,7 @@
#include "Annotations.h"
#include "ClangdServer.h"
+#include "Protocol.h"
#include "SemanticHighlighting.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -66,7 +67,7 @@
}
}
-TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
public:
std::atomic Count = {0};
@@ -90,6 +91,29 @@
ASSERT_EQ(DiagConsumer.Count, 1);
}
+TEST(SemanticHighlighting, toSemanticHighlightingInformation) {
+ auto CreatePosition = [](int Line, int Character) -> Position {
+Position Pos;
+Pos.line = Line;
+Pos.character = Character;
+return Pos;
+ };
+
+ std::vector Tokens{
+ {HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+ {HighlightingKind::Function,
+Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
+ {HighlightingKind::Variable,
+Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+ std::vector ActualResults =
+ toSemanticHighlightingInformation(Tokens);
+ std::vector ExpectedResults = {
+ {1, "AQAEAAA="},
+ {3, "CAAEAAAEAAMAAQ=="}};
+ EXPECT_EQ(ActualResults, ExpectedResults);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -0,0 +1,21 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"semanticHighlightingCapabilities":{"semanticHighlighting":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"int x = 2;"}}}
+# CHECK: "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT:"lines": [
+# CHECK-NEXT: {
+# CHECK-NEXT:"line": 0,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT: }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT: "uri": "file:///clangd-test/foo.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -1,6 +1,6 @@
# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
# Test initialize request parameters with rootUri
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{},"trace":"off"}}
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{"textDocument":{"semanticHighlightingCapabilities":{"semanticHighlighting":true}}},"trace":"off"}}
# CHECK: "id": 0,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": {
@@ -33,6 +33,16 @@
# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "referencesProvider": true,
# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
+# CHECK-NEXT: [
+# CHECK-NEXT:"variable.cpp"
+# CHECK-NEXT: ],
+# CHECK-NEXT: [
+# CHECK-NEXT:"entity.name.function.cpp"
+# CHECK-NEXT: ]
+# CHECK-NEXT:]
+# CHECK-NEXT: },
# CHECK-NEXT: "signatureHelpProvider": {
# CHECK-NEXT:
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
hokein added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:409
}},
+{"semanticHighlighting",
+ llvm::json::Object{{"scopes", getTextMateScopeLookupTable()}}},
The proposal says `If the client declares its capabilities with respect to the
semantic highlighting feature, and if the server supports this feature too, the
server should set all the available TextMate scopes as a "lookup table" during
the initialize request.`.
now it seems that clangd always emit this lookup table to the client.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:144
+ write32be(Token.R.start.character, OS);
+ write16be(Token.R.end.character - Token.R.start.character, OS);
+ write16be(static_cast(Token.Kind), OS);
jvikstrom wrote:
> hokein wrote:
> > if the token is across multiple line, we will emit an ill-formed results.
> There's a FIXME above (which is where it should probably be handled). A bit
> unsure how to solve though. If a token is a block comment spanning multiple
> lines we would need to know the length of every line in the block comment.
> Probably something that can be solved with the ASTContext or SourceManager
> but that can't be accessed in this function.
oh, i missed that FIXME, the FIXME is a bit far away, maybe move it here (now
we assume the token is always on the same line).
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:74
+// FIXME: Factor this out into llvm/Support?
+std::string encodeBase64(const llvm::SmallVectorImpl &U) {
+ static const char Table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
The name `U` doesn't provide much information, maybe `Bytes`?
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:138
+ for (const auto &Line : TokenLines) {
+llvm::SmallVector LineHighlights;
+llvm::raw_svector_ostream OS(LineHighlights);
The code is a bit tricky here, if I understand the code correctly,
`LineHighlights` is the binary data of tokens (each `char` represents a byte).
Maybe `LineEncodingTokens`?
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:141
+for (const auto &Token : Line.second) {
+ // First char is the start column on the line.
+ write32be(Token.R.start.character, OS);
don't repeat what the code does in the comment. Here we are encoding a `Token`
in a form specified in the proposal, I think we should have high-level comment.
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:109
+Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+ std::vector Info =
+ toSemanticHighlightingInformation(Tokens);
nit: ActualResults?
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:111
+ toSemanticHighlightingInformation(Tokens);
+ std::vector Correct = {
+ {1, "AQAEAAA="},
nit: ExpectedResults?
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:114
+ {3, "CAAEAAAEAAMAAQ=="}};
+ ASSERT_EQ(Info, Correct);
+}
nit: we should use `EXPECT_*` when comparing the expected results and actual
results.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom updated this revision to Diff 207714.
jvikstrom added a comment.
Removed unused headers
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/test/semantic-highlighting.test
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -8,6 +8,7 @@
#include "Annotations.h"
#include "ClangdServer.h"
+#include "Protocol.h"
#include "SemanticHighlighting.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -66,7 +67,7 @@
}
}
-TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
public:
std::atomic Count = {0};
@@ -90,6 +91,29 @@
ASSERT_EQ(DiagConsumer.Count, 1);
}
+TEST(SemanticHighlighting, toSemanticHighlightingInformation) {
+ auto CreatePosition = [](int Line, int Character) -> Position {
+Position Pos;
+Pos.line = Line;
+Pos.character = Character;
+return Pos;
+ };
+
+ std::vector Tokens{
+ {HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+ {HighlightingKind::Function,
+Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
+ {HighlightingKind::Variable,
+Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+ std::vector Info =
+ toSemanticHighlightingInformation(Tokens);
+ std::vector Correct = {
+ {1, "AQAEAAA="},
+ {3, "CAAEAAAEAAMAAQ=="}};
+ ASSERT_EQ(Info, Correct);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -0,0 +1,21 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument": {"semanticHighlightingCapabilities": {"semanticHighlighting": true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"int x = 2;"}}}
+# CHECK: "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT:"lines": [
+# CHECK-NEXT: {
+# CHECK-NEXT:"line": 0,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT: }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT: "uri": "file:///clangd-test/foo.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,16 @@
# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "referencesProvider": true,
# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
+# CHECK-NEXT: [
+# CHECK-NEXT:"variable.cpp"
+# CHECK-NEXT: ],
+# CHECK-NEXT: [
+# CHECK-NEXT:"entity.name.function.cpp"
+# CHECK-NEXT: ]
+# CHECK-NEXT:]
+# CHECK-NEXT: },
# CHECK-NEXT: "signatureHelpProvider": {
# CHECK-NEXT:"triggerCharacters": [
# CHECK-NEXT: "(",
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -4,19 +4,25 @@
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
+// An implementation of semantic highlighting based on this proposal:
+// https://github.com/microsoft/vscode-languageserver-node/pull/367 in clangd.
+// Semantic highlightings are calculated for an AST by visiting every AST node
+// and classifying nodes that are interesti
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom updated this revision to Diff 207563.
jvikstrom added a comment.
Removed from hiding under hidden flag .
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/test/semantic-highlighting.test
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -8,6 +8,7 @@
#include "Annotations.h"
#include "ClangdServer.h"
+#include "Protocol.h"
#include "SemanticHighlighting.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -66,7 +67,7 @@
}
}
-TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
public:
std::atomic Count = {0};
@@ -90,6 +91,29 @@
ASSERT_EQ(DiagConsumer.Count, 1);
}
+TEST(SemanticHighlighting, toSemanticHighlightingInformation) {
+ auto CreatePosition = [](int Line, int Character) -> Position {
+Position Pos;
+Pos.line = Line;
+Pos.character = Character;
+return Pos;
+ };
+
+ std::vector Tokens{
+ {HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+ {HighlightingKind::Function,
+Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
+ {HighlightingKind::Variable,
+Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+ std::vector Info =
+ toSemanticHighlightingInformation(Tokens);
+ std::vector Correct = {
+ {1, "AQAEAAA="},
+ {3, "CAAEAAAEAAMAAQ=="}};
+ ASSERT_EQ(Info, Correct);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -0,0 +1,21 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument": {"semanticHighlightingCapabilities": {"semanticHighlighting": true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"int x = 2;"}}}
+# CHECK: "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT:"lines": [
+# CHECK-NEXT: {
+# CHECK-NEXT:"line": 0,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT: }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT: "uri": "file:///clangd-test/foo.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,16 @@
# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "referencesProvider": true,
# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
+# CHECK-NEXT: [
+# CHECK-NEXT:"variable.cpp"
+# CHECK-NEXT: ],
+# CHECK-NEXT: [
+# CHECK-NEXT:"entity.name.function.cpp"
+# CHECK-NEXT: ]
+# CHECK-NEXT:]
+# CHECK-NEXT: },
# CHECK-NEXT: "signatureHelpProvider": {
# CHECK-NEXT:"triggerCharacters": [
# CHECK-NEXT: "(",
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -4,19 +4,25 @@
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
+// An implementation of semantic highlighting based on this proposal:
+// https://github.com/microsoft/vscode-languageserver-node/pull/367 in clangd.
+// Semantic highlightings are calculated for an AST by visiting every AST node
+// and classifying nodes t
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom added inline comments.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:144
+ write32be(Token.R.start.character, OS);
+ write16be(Token.R.end.character - Token.R.start.character, OS);
+ write16be(static_cast(Token.Kind), OS);
hokein wrote:
> if the token is across multiple line, we will emit an ill-formed results.
There's a FIXME above (which is where it should probably be handled). A bit
unsure how to solve though. If a token is a block comment spanning multiple
lines we would need to know the length of every line in the block comment.
Probably something that can be solved with the ASTContext or SourceManager but
that can't be accessed in this function.
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:104
+ HighlightingToken{HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+ HighlightingToken{HighlightingKind::Function,
hokein wrote:
> `Range{ /*start*/{3, 8}, /*end*/{3, 12} }` should be compilable.
Doesn't compile because of the default initialization of `line` and `character`
in Position.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom updated this revision to Diff 207537.
jvikstrom marked 8 inline comments as done.
jvikstrom added a comment.
Address comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/test/semantic-highlighting.test
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -8,6 +8,7 @@
#include "Annotations.h"
#include "ClangdServer.h"
+#include "Protocol.h"
#include "SemanticHighlighting.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -66,7 +67,7 @@
}
}
-TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
public:
std::atomic Count = {0};
@@ -90,6 +91,29 @@
ASSERT_EQ(DiagConsumer.Count, 1);
}
+TEST(SemanticHighlighting, toSemanticHighlightingInformation) {
+ auto CreatePosition = [](int Line, int Character) -> Position {
+Position Pos;
+Pos.line = Line;
+Pos.character = Character;
+return Pos;
+ };
+
+ std::vector Tokens{
+ {HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+ {HighlightingKind::Function,
+Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
+ {HighlightingKind::Variable,
+Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+ std::vector Info =
+ toSemanticHighlightingInformation(Tokens);
+ std::vector Correct = {
+ {1, "AQAEAAA="},
+ {3, "CAAEAAAEAAMAAQ=="}};
+ ASSERT_EQ(Info, Correct);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -0,0 +1,21 @@
+# RUN: clangd -hidden-features -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument": {"semanticHighlightingCapabilities": {"semanticHighlighting": true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"int x = 2;"}}}
+# CHECK: "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT:"lines": [
+# CHECK-NEXT: {
+# CHECK-NEXT:"line": 0,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT: }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT: "uri": "file:///clangd-test/foo.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,16 @@
# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "referencesProvider": true,
# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
+# CHECK-NEXT: [
+# CHECK-NEXT:"variable.cpp"
+# CHECK-NEXT: ],
+# CHECK-NEXT: [
+# CHECK-NEXT:"entity.name.function.cpp"
+# CHECK-NEXT: ]
+# CHECK-NEXT:]
+# CHECK-NEXT: },
# CHECK-NEXT: "signatureHelpProvider": {
# CHECK-NEXT:"triggerCharacters": [
# CHECK-NEXT: "(",
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -4,19 +4,25 @@
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
+// An implementation of semantic highlighting based on this proposal:
+// https://github.com/microsoft/vscode-languageserver-node/pull/367 in clangd.
+// Semantic highlightings are calculated for an AST by visiting eve
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
hokein added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:333
+ ClangdServerOpts.SemanticHighlighting =
+ Params.capabilities.SemanticHighlighting;
if (Params.rootUri && *Params.rootUri)
I'd put the logic here rather than in `ClangdServer.cpp`, just
`ClangdServerOpts.SemanticHighlighting = ClangdServerOpts.HiddenFeatures &&
Params.capabilities.SemanticHighlighting;`.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:131
+if (TokenLines.find(LineIdx) == TokenLines.end()) {
+ TokenLines[LineIdx] = {Token};
+} else {
here we lookup the map twice, we could save one cost of lookup.
`TokenLines[LindIdx].push_back(Token)` should work.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:143
+for (const auto &Token : Line.second) {
+ write32be(Token.R.start.character, OS);
+ write16be(Token.R.end.character - Token.R.start.character, OS);
could we have some comments explaining the details about encoding Token here?
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:144
+ write32be(Token.R.start.character, OS);
+ write16be(Token.R.end.character - Token.R.start.character, OS);
+ write16be(static_cast(Token.Kind), OS);
if the token is across multiple line, we will emit an ill-formed results.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:44
+
+// Converts a vector of HighlightingTokens to a vector of
+// SemanticHighlightingInformation. The token string in
just: convert to LSP's semantic highlighting information.
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:103
+ std::vector Tokens{
+ HighlightingToken{HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
we could drop the explicit `HighlightingToken`, just
```
... Tokens = {
{HighlightingKind::Variable, Range {...}},
{}
}
```
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:104
+ HighlightingToken{HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+ HighlightingToken{HighlightingKind::Function,
`Range{ /*start*/{3, 8}, /*end*/{3, 12} }` should be compilable.
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:112
+ std::vector Correct = {
+ SemanticHighlightingInformation{1, "AQAEAAA="},
+ SemanticHighlightingInformation{3, "CAAEAAAEAAMAAQ=="}};
here as well, just `{ {1, "" } }`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom updated this revision to Diff 207241.
jvikstrom marked 13 inline comments as done.
jvikstrom added a comment.
Addressed comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/test/semantic-highlighting.test
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -8,6 +8,7 @@
#include "Annotations.h"
#include "ClangdServer.h"
+#include "Protocol.h"
#include "SemanticHighlighting.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -66,7 +67,7 @@
}
}
-TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
public:
std::atomic Count = {0};
@@ -90,6 +91,29 @@
ASSERT_EQ(DiagConsumer.Count, 1);
}
+TEST(SemanticHighlighting, toSemanticHighlightingInformation) {
+ auto CreatePosition = [](int Line, int Character) -> Position {
+Position Pos;
+Pos.line = Line;
+Pos.character = Character;
+return Pos;
+ };
+
+ std::vector Tokens{
+ HighlightingToken{HighlightingKind::Variable,
+Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+ HighlightingToken{HighlightingKind::Function,
+Range{CreatePosition(3, 4), CreatePosition(3, 7)}},
+ HighlightingToken{HighlightingKind::Variable,
+Range{CreatePosition(1, 1), CreatePosition(1, 5)}}};
+ std::vector Info =
+ toSemanticHighlightingInformation(Tokens);
+ std::vector Correct = {
+ SemanticHighlightingInformation{1, "AQAEAAA="},
+ SemanticHighlightingInformation{3, "CAAEAAAEAAMAAQ=="}};
+ ASSERT_EQ(Info, Correct);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -0,0 +1,21 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument": {"semanticHighlightingCapabilities": {"semanticHighlighting": true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"int x = 2;"}}}
+# CHECK: "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT:"lines": [
+# CHECK-NEXT: {
+# CHECK-NEXT:"line": 0,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT: }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT: "uri": "file:///clangd-test/foo.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,16 @@
# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "referencesProvider": true,
# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
+# CHECK-NEXT: [
+# CHECK-NEXT:"variable.cpp"
+# CHECK-NEXT: ],
+# CHECK-NEXT: [
+# CHECK-NEXT:"entity.name.function.cpp"
+# CHECK-NEXT: ]
+# CHECK-NEXT:]
+# CHECK-NEXT: },
# CHECK-NEXT: "signatureHelpProvider": {
# CHECK-NEXT:"triggerCharacters": [
# CHECK-NEXT: "(",
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -4,19 +4,25 @@
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
+// An implementation of semantic highlighting based on this proposal:
+// h
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
hokein added inline comments.
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:332
+ ClangdServerOpts.SemanticHighlighting =
+ Params.capabilities.SemanticHighlighting;
I'm a bit nervous -- we will turn on this feature when the client states that
it supports semantic highlighting, client may disable its reg-based
highlighting, and relies on clangd's, but the feature in clangd is in pretty
early stage...
I think we'd need to add a new command-line flag to clangd (disabled by
default).
Comment at: clang-tools-extra/clangd/Protocol.h:1179
+// Contains all highlightings in a single line.
+struct SemanticHighlightingInformation {
please use `///` which is doxygen comment.
And `/// Represents a semantic highlighting information that has to be applied
on a specific line of the text document.`
Comment at: clang-tools-extra/clangd/Protocol.h:1182
+ // The line these highlightings belong to.
+ unsigned int Line;
+ // The base64 encoded string of highlighting tokens.
nit: using `int` is fine here.
Comment at: clang-tools-extra/clangd/Protocol.h:1189
+
+struct SemanticHighlightingParams {
+ // The textdocument these highlightings belong to.
Add a comment: `/// Parameters for the semantic highlighting (server-side) push
notification.`
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:120
+std::vector
+highlightingTokensToSemanticHighlightingInformation(
+llvm::ArrayRef Tokens) {
add unittests for this function?
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:132
+ llvm::raw_svector_ostream OS(LineHighlights);
+ for (const auto &Token : Tokens) {
+if (Token.R.start.line != LastLine) {
the implementation requires Tokens sorted, maybe consider using a map to the
group-by?
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:152
+ std::map> Scopes = {
+ {HighlightingKind::Variable, {"variable.cpp"}},
+ {HighlightingKind::Function, {"entity.name.function.cpp"}}};
thinking more about this, the `.cpp` suffix is language-specific, clangd also
supports other C-family languages (e.g. C, ObjC), so we may need corresponding
language suffix in the scopes (based on the language mode).
No need to do the change here, but please add a FIXME.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:8
//===--===//
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHTING_H
The semantic highlighting is not in LSP yet, I think we need some
documentations here to explain some more details about this feature in clangd,
like the implementations are based on the proposal
(https://github.com/microsoft/vscode-languageserver-node/pull/367).
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:19
enum class HighlightingKind {
Variable,
Function,
as this kind is used as the lookup table index, let's add `Variable = 0`
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:37
+// SemanticHighlightKind indexes correctly into this vector.
+std::vector> getSemanticTextMateScopes();
+
I'd name it `textMateScopeLookupTable`
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:40
+// Converts a vector of HighlightingTokens to a vector of
+// SemanticHighlightingInformation. Assumes the HighlightingToken's vector is
+// ordered in ascending order by line and secondarily by column of the token's
the assumption seems too strict here. IIUC, LSP doesn't require the order of
the `SemanticHighlightingInformation[]`, would be nice to return an ordered one
(but that's implementation detail).
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:46
+std::vector
+highlightingTokensToSemanticHighlightingInformation(
+llvm::ArrayRef Tokens);
just `toSemanticHighlightingInformation`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom updated this revision to Diff 207031.
jvikstrom marked an inline comment as done.
jvikstrom added a comment.
Added SemanticHighlightingInformation and SemantigHighlightingParams.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/test/semantic-highlighting.test
Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -0,0 +1,21 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument": {"semanticHighlightingCapabilities": {"semanticHighlighting": true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"int x = 2;"}}}
+# CHECK: "method": "textDocument/semanticHighlighting",
+# CHECK-NEXT: "params": {
+# CHECK-NEXT:"lines": [
+# CHECK-NEXT: {
+# CHECK-NEXT:"line": 0,
+# CHECK-NEXT:"tokens": "BAABAAA="
+# CHECK-NEXT: }
+# CHECK-NEXT:],
+# CHECK-NEXT:"textDocument": {
+# CHECK-NEXT: "uri": "file:///clangd-test/foo.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT: }
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,16 @@
# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "referencesProvider": true,
# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
+# CHECK-NEXT: [
+# CHECK-NEXT:"variable.cpp"
+# CHECK-NEXT: ],
+# CHECK-NEXT: [
+# CHECK-NEXT:"entity.name.function.cpp"
+# CHECK-NEXT: ]
+# CHECK-NEXT:]
+# CHECK-NEXT: },
# CHECK-NEXT: "signatureHelpProvider": {
# CHECK-NEXT:"triggerCharacters": [
# CHECK-NEXT: "(",
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHTING_H
#include "ClangdUnit.h"
+#include "Protocol.h"
namespace clang {
namespace clangd {
@@ -31,6 +32,20 @@
// main AST.
std::vector getSemanticHighlightings(ParsedAST &AST);
+// Gets the TextMate scopes as a double nested array where the
+// SemanticHighlightKind indexes correctly into this vector.
+std::vector> getSemanticTextMateScopes();
+
+// Converts a vector of HighlightingTokens to a vector of
+// SemanticHighlightingInformation. Assumes the HighlightingToken's vector is
+// ordered in ascending order by line and secondarily by column of the token's
+// start position. The returned SemanticHighlightingInformation vector will be
+// ordered in ascending order according to the line number. The token string in
+// SemanticHighlightingInformation is base64 encoded.
+std::vector
+highlightingTokensToSemanticHighlightingInformation(
+llvm::ArrayRef Tokens);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -8,6 +8,7 @@
#include "SemanticHighlighting.h"
#include "Logger.h"
+#include "Protocol.h"
#include "SourceCode.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/RecursiveASTVisitor.h"
@@ -63,6 +64,48 @@
}
};
+// Encode binary data into base64.
+// This was copied from compiler-rt/lib/fuzzer/FuzzerUtil.cpp.
+// FIXME: Factor this out into llvm/Support?
+std::string encodeBase64(const llvm::SmallVectorImpl &U) {
+ static const char Table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+ "abcdefghijklmnopqrstuvwxyz"
+ "0123456789+/";
+ std::string Res;
+ size_t I;
+ for (I = 0; I + 2 < U.size(); I += 3) {
+uint32_t X = (U[I] << 16) + (U[I + 1] << 8) + U[I + 2];
+Res += Table[(X >> 18) & 63];
+Res += Table[(X >>
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
hokein added inline comments.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:31
+// Contains all highlightings in a single line.
+struct LineHighlighting {
+ LineHighlighting(unsigned int Line = 0,
The structures defined here are similar to the structures proposed in LSP
`SemanticHighlightingParams` and `SemanticHighlightingInformation`, we could
define these structures in the `Protocol.h` (you can find similar pattern
there).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63919/new/
https://reviews.llvm.org/D63919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled
jvikstrom created this revision.
jvikstrom added reviewers: hokein, kadircet.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
Emit publishSemanticHighlighting in LSP if enabled
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D63919
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/SemanticHighlighting.h
clang-tools-extra/clangd/test/initialize-params.test
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,16 @@
# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "referencesProvider": true,
# CHECK-NEXT: "renameProvider": true,
+# CHECK-NEXT: "semanticHighlighting": {
+# CHECK-NEXT:"scopes": [
+# CHECK-NEXT: [
+# CHECK-NEXT:"variable.cpp"
+# CHECK-NEXT: ],
+# CHECK-NEXT: [
+# CHECK-NEXT:"entity.name.function.cpp"
+# CHECK-NEXT: ]
+# CHECK-NEXT:]
+# CHECK-NEXT: },
# CHECK-NEXT: "signatureHelpProvider": {
# CHECK-NEXT:"triggerCharacters": [
# CHECK-NEXT: "(",
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -27,10 +27,34 @@
bool operator==(const HighlightingToken &Lhs, const HighlightingToken &Rhs);
+// Contains all highlightings in a single line.
+struct LineHighlighting {
+ LineHighlighting(unsigned int Line = 0,
+ std::vector Tokens = {})
+ : Line(Line), Tokens(Tokens) {}
+ unsigned int Line;
+ std::vector Tokens;
+};
+
+llvm::json::Value toJSON(const LineHighlighting &Highlighting);
+
// Returns all HighlightingTokens from an AST. Only generates highlights for the
// main AST.
std::vector getSemanticHighlightings(ParsedAST &AST);
+// Gets the TextMate scopes as a double nested array where the
+// SemanticHighlightKind indexes correctly into this vector.
+std::vector> getSemanticTextMateScopes();
+
+// Converts a vector of HighlightingTokens to a vector of LineHighlightings.
+// Assumes the HighlightingToken's vector is ordered in ascending order by line
+// and secondarily by column of the token's start position. The returned
+// LineHighlighting vector will be ordered in ascending order according to the
+// line number. The tokens in a LineHighlighting are ordered in ascending order
+// according to the token's start character.
+std::vector
+highlightingTokensToLines(llvm::ArrayRef Tokens);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -63,8 +63,69 @@
}
};
+// Encode binary data into base64.
+// This was copied from compiler-rt/lib/fuzzer/FuzzerUtil.cpp.
+// FIXME: Factor this out into llvm/Support?
+std::string encodeBase64(const llvm::SmallVectorImpl &U) {
+ static const char Table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+ "abcdefghijklmnopqrstuvwxyz"
+ "0123456789+/";
+ std::string Res;
+ size_t I;
+ for (I = 0; I + 2 < U.size(); I += 3) {
+uint32_t X = (U[I] << 16) + (U[I + 1] << 8) + U[I + 2];
+Res += Table[(X >> 18) & 63];
+Res += Table[(X >> 12) & 63];
+Res += Table[(X >> 6) & 63];
+Res += Table[X & 63];
+ }
+ if (I + 1 == U.size()) {
+uint32_t X = (U[I] << 16);
+Res += Table[(X >> 18) & 63];
+Res += Table[(X >> 12) & 63];
+Res += "==";
+ } else if (I + 2 == U.size()) {
+uint32_t X = (U[I] << 16) + (U[I + 1] << 8);
+Res += Table[(X >> 18) & 63];
+Res += Table[(X >> 12) & 63];
+Res += Table[(X >> 6) & 63];
+Res += "=";
+ }
+ return Res;
+}
+
+void write32be(uint32_t I, llvm::raw_ostream &OS) {
+ std::array Buf;
+ llvm::support::endian::write32be(Buf.data(), I);
+ OS.write(Buf.data(), Buf.size());
+}
+
+void write16be(uint16_t I, llvm::raw_ostream &OS) {
+ std::array Buf;
+ llvm::support::endian::write16be(Buf.data(), I);
+ OS.write(Buf.data(), Buf.size());
+}
} // namespace
+llvm::json::Value toJSON(const LineHighlighting &Highlight) {
+ llvm::SmallVector BinaryHighlights;
+ llvm::raw_svector_ostream OS(BinaryHighlights);
+
+ for (size_t I = 0, End = Highlight.Tokens.size(); I < End; I++) {
+// Each token should consists of 2 32 bit integers. The first integer is the
+// start column of the token.
