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<char, 128> 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<HighlightingToken> 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<std::vector<std::string>> NestedScopes(Scopes.size());
+ for (const auto &Scope : Scopes) {
+ NestedScopes[static_cast<int>(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
